fix(pip): do not add a pip-fallback when there is no sdist #3432
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.
| "direct_sdist_without_sha", | ||
| "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.
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