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

Enable support for source distribution if whl is compatible #2354

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

WeishiZ
Copy link

@WeishiZ WeishiZ commented Mar 17, 2025

Changes in conda_environment.py

For source distribution file format, url would end with .tar.gz.
Existing logic in metaflow/plugins/pypi/pip.py would build it into .whl file.
When saving to storage, it should preserve the .whl file extension, so it can be installed by pip later.

Changes in pip.py

Ordered dedup so pip command sees the --platform xxx options in deterministic order every time.
(This can be crucial if any 3rd party dependency made a mistake tagging its wheel. E.g. dm_tree is tagged as compatible with arm64 when its not. In such case, a nondeterministic order would cause intermittent errors hard to debug.)

@WeishiZ WeishiZ marked this pull request as draft March 17, 2025 23:18
@WeishiZ WeishiZ marked this pull request as ready for review March 17, 2025 23:28
@savingoyal savingoyal requested a review from saikonen March 18, 2025 01:34
@@ -78,7 +78,7 @@ def solve(self, id_, packages, python, platform):
cmd = [
"install",
"--dry-run",
"--only-binary=:all:", # only wheels
"--prefer-binary",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure that this works -

command '/Users/savin/.metaflowconfig/micromamba/bin/micromamba run --prefix /Users/savin/micromamba/envs/metaflow/osx-arm64/c9a70c6534aed5f pip3 --disable-pip-version-check --no-color --no-input --isolated install --dry-run --prefer-binary --upgrade-strategy=only-if-needed --target=/var/folders/jt/vd16shsx4ts47bvv4fnwscmc0000gn/T/tmp2vb9d3fx --report=/var/folders/jt/vd16shsx4ts47bvv4fnwscmc0000gn/T/tmp2vb9d3fx/report.json --progress-bar=off --quiet --abi cp313 --abi abi3 --abi none --platform macosx_15_0_arm64 --platform macosx_15_0_universal2 --platform macosx_14_0_arm64 --platform macosx_14_0_universal2 --platform macosx_13_0_arm64 --platform macosx_13_0_universal2 --platform macosx_12_0_arm64 --platform macosx_12_0_universal2 --platform macosx_11_0_arm64 --platform macosx_11_0_universal2 --platform macosx_10_16_universal2 --platform macosx_10_15_universal2 --platform macosx_10_14_universal2 --platform macosx_10_13_universal2 --platform macosx_10_12_universal2 --platform macosx_10_11_universal2 --platform macosx_10_10_universal2 --platform macosx_10_9_universal2 --platform macosx_10_8_universal2 --platform macosx_10_7_universal2 --platform macosx_10_6_universal2 --platform macosx_10_5_universal2 --platform macosx_10_4_universal2 --platform any requests>=2.21.0 boto3>=1.14.0 fire==0.7.0' returned error (1)
    ERROR: When restricting platform and interpreter constraints using --python-version, --platform, --abi, or --implementation, either --no-deps must be set, or --only-binary=:all: must be set and --no-binary must not be set (or must be set to :none:).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the issue now. This is indeed a problem.
In our internal fork we have --no-deps added that made this work. We have --no-deps since we use poetry to resolve dependencies first and then copy the resolved ones to @pypi. (will send you more details on the internal plugin in a separate thread.)

This error suggests that pip install --platform xxx --abi xxx has to use --no-deps if want to support non-binary.

ERROR: When restricting platform and interpreter constraints using --python-version, --platform, --abi, or --implementation, either --no-deps must be set, or --only-binary=:all: must be set and --no-binary must not be set (or must be set to :none:).

I'll need to explore more to understand why pip is restrictive about building transitive deps from source.

Possible direction could be to use a separate standalone resolver, or remove the pip platform options (--platform xxx --abi xxx) if it's a sdist. As long as it can ensure the built wheels are correctly tagged, then pip.py will decide if it's compatible with the target platform.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, neither of those approaches will work while solving for the environment. There is a possibility to make it work when the local_platform is the same as the target_platform - but outside of that - for cross-platform or cross-python solves, we can't really solve it by depending on pip alone - it is tricky because one needs to build the source to generate downstream dependency graph - which may not be always possible. I am hopeful that there is some approach that allows us to include well-behaved source distributions, given that git dependencies work well already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants