Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pip.parse(python_version=) should not be mandatory #1708

Open
mering opened this issue Jan 19, 2024 · 5 comments
Open

pip.parse(python_version=) should not be mandatory #1708

mering opened this issue Jan 19, 2024 · 5 comments
Assignees
Milestone

Comments

@mering
Copy link

mering commented Jan 19, 2024

🐞 bug report

Affected Rule

Bzlmod pip.parse()

Is this a regression?

Yes, was not a problem in WORKSPACE.

Description

The arg documentation says the following:

The Python version the dependencies are targetting, in Major.Minor format
(e.g., "3.11"). Patch level granularity (e.g. "3.11.1") is not supported.
If not specified, then the default Python version (as set by the root module or
rules_python) will be used.
If an interpreter isn't explicitly provided (using `python_interpreter` or
`python_interpreter_target`), then the version specified here must have
a corresponding `python.toolchain()` configured.

It specifically describes If not specified, then the default Python version (as set by the root module or rules_python) will be used. So this attribute should be optional but currently is mandatory.

🔬 Minimal Reproduction

pip.parse() without python_version.

🔥 Exception or Error

ERROR: in tag at <root>/MODULE.bazel:42:10, mandatory attribute python_version isn't being specified

🌍 Your Environment

Operating System:

  
Linux
  

Output of bazel version:

  
2024/01/19 15:28:42 Warning: used fallback version "6.3.2"
Bazelisk version: v1.19.0
INFO: Running bazel wrapper (see //tools/bazel for details), bazel version 6.1.2 will be used instead of system-wide bazel installation.
Build label: 6.1.2
Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Tue Apr 18 15:29:54 2023 (1681831794)
Build timestamp: 1681831794
Build timestamp as int: 1681831794
  

Rules_python version:

  
0.27.1
  

Anything else relevant?

@aignas
Copy link
Collaborator

aignas commented Jan 24, 2024

Could you explain a little more how having a requirement on this mandatory argument make it more difficult to use rules_python? Are there cases where you cannot do this? Usually requirements.txt files are python version and platform dependent, so I'd be curious to hear out more about your usecase.

@mering
Copy link
Author

mering commented Jan 26, 2024

Could you explain a little more how having a requirement on this mandatory argument make it more difficult to use rules_python? Are there cases where you cannot do this? Usually requirements.txt files are python version and platform dependent, so I'd be curious to hear out more about your usecase.

When you create a module which uses a requirements.txt but is meant to be depended upon by another module, my module doesn't know with which version the dependent module wants to use. So I have to run this for all available Python versions. If I could instead just leave this out and have it determined automatically by the action graph which version is required, it would be much easier to use.

@lalten
Copy link
Contributor

lalten commented May 21, 2024

Did you find a solution for this? https://github.com/mvukov/rules_ros2/blob/1e16de8c3797d94b54ea589524fa7be0e45481d9/MODULE.bazel#L34-L67 is a workaround, but I hate it :P

@aignas
Copy link
Collaborator

aignas commented Jun 1, 2024 via email

@mering
Copy link
Author

mering commented Jun 1, 2024

I am curious of the usecase that you are trying to cover by using all of the Python versions and requirements. Is it for testing that your dependencies work on all python versions or is it something else?

Isn't this required when you don't know which Python version other modules which depend on your module are using?

Let's say, I develop module X which has pip dependencies. When module A wants to depend on X and uses Python version 3.8, this version needs to be declared in X's pip_parse(). When module B wants to depend on X and uses Python version 3.11, this version also needs to be declared in X's pip_parse(). As a developer of module X I don't know which Python versions the users of my module want to use, so I need to declare all of them at the moment. It would be preferable if I would only need to declare my requirements.txt file and have Bazel figure out the correct Python version to use in module X based on the toolchains used in dependent modules.

github-merge-queue bot pushed a commit that referenced this issue Jul 20, 2024
It seems that a few things broke in recent commits:
- We are not using the `MODULE.bazel.lock` file and it seems that it is
easy to
  miss when the components in the PyPI extension stop integrating well
together. This happened during the switch to `{abi}_{os}_{plat}` target
  platform passing within the code.
- The logger code stopped working in the extension after the recent
additions
  to add the `rule_name`.
- `repo_utils.getenv` was always getting `PATH` env var on bazel `6.x`.

This PR fixes both cases and updates docs to serve as a better reminder.
By
fixing the `select_whls` code and we can just rely on target platform
triples
(ABI, OS, CPU). This gets one step closer to maybe supporting optional
`python_version` which would address #1708. Whilst at it we are also
adding
different status messages for building the wheel from `sdist` vs just
extracting or downloading the wheel.

Tests:
- Added more unit tests and brought them in line with the rest of the
code.
- Checked manually for differences between the `MODULE.bazel.lock` files
in our
`rules_python` extension before #2069 and after this PR and there are no
  differences except in the `experimental_target_platforms` attribute in
`whl_library`. Before this PR you would see that we do not select any
wheels
  for e.g. `MarkupSafe` and we are always building from `sdist`.

Work towards #260.
@aignas aignas self-assigned this Jul 23, 2024
@aignas aignas added this to the v1.0.0 milestone Jul 23, 2024
github-merge-queue bot pushed a commit that referenced this issue Oct 7, 2024
With this change we set the default value of `--python_version` when
the `python.toolchain` is used in `bzlmod` and we generate the
appropriate config settings based on the registered toolchains and
given overrides by the root module.

This means that we expect the `--python_version` to be always set to
a non-empty value under `bzlmod` and we can cleanup code which was
handling `//conditions:default` case. This also means that we can
in theory drop the requirement for `python_version` in `pip.parse`
and just setup dependencies for all packages that we find in the
`requirements.txt` file and move on. This is left as future work
by myself or anyone willing to contribute. We can also start reusing
the same `whl_library` instance for multi-platform packages because
the python version flag is always set - this will simplify the layout
and makes the feature non-experimental anymore under bzlmod.

Summary:
* Add `@pythons_hub` to the `WORKSPACE` with dummy data to make
  pythons_hub work.
* Add `MINOR_MAPPING` and `PYTHON_VERSIONS` to pythons_hub to
  generate the config settings.
* Remove handling of the default version in `pypi` code under bzlmod.

Work towards #2081, #260, #1708

---------

Co-authored-by: Richard Levasseur <rlevasseur@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants