fix(pip): do not add a pip-fallback when there is no sdist#3432
fix(pip): do not add a pip-fallback when there is no sdist#3432rickeylev merged 9 commits intobazel-contrib:mainfrom
Conversation
Summary of ChangesHello @aignas, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the dependency resolution logic by preventing the system from attempting a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
aignas
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the pip fallback logic to correctly avoid falling back when no source distribution (sdist) is available. This is a good improvement as it removes the need for the download = True workaround and makes the behavior more intuitive.
The changes in python/private/pypi/parse_requirements.bzl are clean, introducing a can_fallback flag from _add_dists to control the fallback behavior. The logic sdist != None correctly determines if a fallback is possible.
The test updates in tests/pypi/hub_builder/hub_builder_tests.bzl are comprehensive. They correctly test the new behavior by adding cases with wheels but no sdists, and by removing the download_only = True workaround. The cleanup of unused marker stubs is also a welcome improvement.
I have a couple of suggestions for minor improvements: one for code simplification in parse_requirements.bzl and another to correct a test assertion in hub_builder_tests.bzl.
| @@ -873,6 +881,7 @@ git_dep @ git+https://git.server/repo/project@deadbeefdeadbeef | |||
| "direct_without_sha", | |||
| "git_dep", | |||
| "pip_fallback", | |||
There was a problem hiding this comment.
The package plat_pkg should probably not be in the exposed_packages list. A package is considered 'exposed' only if it is available for all target platforms. In this test, plat_pkg provides a wheel only for linux_x86_64 and has no sdist, so it's not available for other platforms like osx_aarch64 or windows_aarch64. Therefore, it should not be exposed and should be removed from this list.
### What does this PR do? Update `rules_python` from version 1.6.3 to 1.8.3 to reduce the number of DEBUG warnings seen in CI, which tends to indicate a combinatorial explosion. ### Motivation Some CI runs generate thousands of warnings like: ``` DEBUG: rules_python:pypi:create_whl_repos WARNING: Could not find a whl or an sdist with sha256=... ``` ([example](https://gitlab.ddbuild.io/DataDog/datadog-agent/-/jobs/1406186495)). While switching to version 1.8.x alone is unlikely to eliminate all warnings, it brings changes that should help reduce their frequency: - bazel-contrib/rules_python#3225 - bazel-contrib/rules_python#3385 - bazel-contrib/rules_python#3441 - bazel-contrib/rules_python#3432 - bazel-contrib/rules_python#3447 ### Additional Notes This is a mitigation/preparation step: if warning counts don't decrease significantly, the next step would be to add `target_platforms` to the `pip.parse()` configuration to explicitly limit platform checks, which is worth a dedicated PR.
### What does this PR do? Update `rules_python` from version 1.6.3 to 1.8.3 to reduce the number of DEBUG warnings seen in CI, which tends to indicate a combinatorial explosion. ### Motivation Some CI runs generate thousands of warnings like: ``` DEBUG: rules_python:pypi:create_whl_repos WARNING: Could not find a whl or an sdist with sha256=... ``` ([example](https://gitlab.ddbuild.io/DataDog/datadog-agent/-/jobs/1406186495)). While switching to version 1.8.x alone is unlikely to eliminate all warnings, it brings changes that should help reduce their frequency: - bazel-contrib/rules_python#3225 - bazel-contrib/rules_python#3385 - bazel-contrib/rules_python#3441 - bazel-contrib/rules_python#3432 - bazel-contrib/rules_python#3447 ### Additional Notes This is a mitigation/preparation step: if warning counts don't decrease significantly, the next step would be to add `target_platforms` to the `pip.parse()` configuration to explicitly limit platform checks, which is worth a dedicated PR.
### What does this PR do? Update `rules_python` from version 1.6.3 to 1.8.3 to reduce the number of DEBUG warnings seen in CI, which tends to indicate a combinatorial explosion. ### Motivation Some CI runs generate thousands of warnings like: ``` DEBUG: rules_python:pypi:create_whl_repos WARNING: Could not find a whl or an sdist with sha256=... ``` ([example](https://gitlab.ddbuild.io/DataDog/datadog-agent/-/jobs/1406186495)). While switching to version 1.8.x alone is unlikely to eliminate all warnings, it brings changes that should help reduce their frequency: - bazel-contrib/rules_python#3225 - bazel-contrib/rules_python#3385 - bazel-contrib/rules_python#3441 - bazel-contrib/rules_python#3432 - bazel-contrib/rules_python#3447 ### Additional Notes This is a mitigation/preparation step: if warning counts don't decrease significantly, the next step would be to add `target_platforms` to the `pip.parse()` configuration to explicitly limit platform checks, which is worth a dedicated PR.
### What does this PR do? Update `rules_python` from version 1.6.3 to 1.8.3 to reduce the number of DEBUG warnings seen in CI, which tends to indicate a combinatorial explosion. ### Motivation Some CI runs generate thousands of warnings like: ``` DEBUG: rules_python:pypi:create_whl_repos WARNING: Could not find a whl or an sdist with sha256=... ``` ([example](https://gitlab.ddbuild.io/DataDog/datadog-agent/-/jobs/1406186495)). While switching to version 1.8.x alone is unlikely to eliminate all warnings, it brings changes that should help reduce their frequency: - bazel-contrib/rules_python#3225 - bazel-contrib/rules_python#3385 - bazel-contrib/rules_python#3441 - bazel-contrib/rules_python#3432 - bazel-contrib/rules_python#3447 ### Additional Notes This is a mitigation/preparation step: if warning counts don't decrease significantly, the next step would be to add `target_platforms` to the `pip.parse()` configuration to explicitly limit platform checks, which is worth a dedicated PR.
### What does this PR do? Update `rules_python` from version 1.6.3 to 1.8.3 to reduce the number of DEBUG warnings seen in CI, which tends to indicate a combinatorial explosion. ### Motivation Some CI runs generate thousands of warnings like: ``` DEBUG: rules_python:pypi:create_whl_repos WARNING: Could not find a whl or an sdist with sha256=... ``` ([example](https://gitlab.ddbuild.io/DataDog/datadog-agent/-/jobs/1406186495)). While switching to version 1.8.x alone is unlikely to eliminate all warnings, it brings changes that should help reduce their frequency: - bazel-contrib/rules_python#3225 - bazel-contrib/rules_python#3385 - bazel-contrib/rules_python#3441 - bazel-contrib/rules_python#3432 - bazel-contrib/rules_python#3447 ### Additional Notes This is a mitigation/preparation step: if warning counts don't decrease significantly, the next step would be to add `target_platforms` to the `pip.parse()` configuration to explicitly limit platform checks, which is worth a dedicated PR.
### What does this PR do? Update `rules_python` from version 1.6.3 to 1.8.3 with the aim at reducing the number of warnings seen in CI, which tend to indicate an underlying combinatorial explosion when looking for Python wheels. ### Motivation Some CI runs generate thousands of warnings like: ``` DEBUG: rules_python:pypi:create_whl_repos WARNING: Could not find a whl or an sdist with sha256=... ``` ([example](https://gitlab.ddbuild.io/DataDog/datadog-agent/-/jobs/1406186495)). While switching to version 1.8.x alone is unlikely to eliminate all warnings, it brings changes that should help reduce their frequency: - bazel-contrib/rules_python#3225 - bazel-contrib/rules_python#3385 - bazel-contrib/rules_python#3441 - bazel-contrib/rules_python#3432 - bazel-contrib/rules_python#3447 ### Describe how you validated your changes For that, I'll be monitoring logs once the change is merged to the mainline, see why below. ### Additional Notes Coming next: - this is a mitigation/preparation step: if warning counts don't decrease significantly, the next step would be to add `target_platforms` to the `pip.parse()` configuration to explicitly limit platform checks, which is worth a dedicated PR, - even if it's tiny, the patch on `rules_python` must be removed once bazel-contrib/rules_python#3579 is addressed. Without it, we used to face `ValueError`s which were preventing the version bump. Co-authored-by: regis.desgroppes <[email protected]>
2 participants
Before this PR the user would have to specify
download = Truein order to notfallback to pip, which is admittedly an odd interface design. With this PR we
correctly do not add a
pipfallback if there is nosdistto be used. Withthis in place we are better placed to enable the
experimental_index_urlbydefault. What is more we can more reliably detect when we should use a special
repository rule to prepare for building from sdist.
Summary:
test.
Work towards #260
Work towards #2410