Skip to content

fix(pip): Pass --abi flag when downloading wheel for freethreaded python#3762

Open
meteorcloudy wants to merge 1 commit intobazel-contrib:mainfrom
meteorcloudy:abi_for_download
Open

fix(pip): Pass --abi flag when downloading wheel for freethreaded python#3762
meteorcloudy wants to merge 1 commit intobazel-contrib:mainfrom
meteorcloudy:abi_for_download

Conversation

@meteorcloudy
Copy link
Copy Markdown
Contributor

When pip download is used for fetching pip dependency, we need to pass --abi flag to fetch the correct wheel file for freethreaded python.

Essentially making https://github.com/google-ml-infra/rules_ml_toolchain/blob/1c2352bde159ee14cc5050a871c437622fe25e1f/py/python_init_pip.bzl#L69-L71 unnecessary.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces logic to automatically append the correct --abi flag for freethreaded Python platforms when download_only is enabled. This is achieved by passing the download_only and python_version attributes through the hub builder to the requirement parser. A review comment suggests optimizing the implementation by avoiding redundant list copies and refining the check for existing --abi flags.

Comment on lines +119 to +126
plat_args = list(pip_args)
if download_only and plat.endswith("freethreaded") and python_version:
if not any([a.startswith("--abi") for a in plat_args]):
major, _, tail = python_version.partition(".")
minor, _, _ = tail.partition(".")
if major and minor:
plat_args.append("--abi=cp{}{}t".format(major, minor))
options[plat] = plat_args
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The current implementation creates a new list copy for every platform, even when no modifications are needed. This can be optimized by only copying the list if the conditions for adding the --abi flag are met. Additionally, the check for existing --abi flags can be made more precise to avoid matching other flags that might start with the same prefix (though unlikely in this context).

Suggested change
plat_args = list(pip_args)
if download_only and plat.endswith("freethreaded") and python_version:
if not any([a.startswith("--abi") for a in plat_args]):
major, _, tail = python_version.partition(".")
minor, _, _ = tail.partition(".")
if major and minor:
plat_args.append("--abi=cp{}{}t".format(major, minor))
options[plat] = plat_args
plat_args = pip_args
if download_only and plat.endswith("freethreaded") and python_version:
if not any([a == "--abi" or a.startswith("--abi=") for a in plat_args]):
major, _, tail = python_version.partition(".")
minor, _, _ = tail.partition(".")
if major and minor:
plat_args = list(pip_args)
plat_args.append("--abi=cp{}{}t".format(major, minor))
options[plat] = plat_args

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.

1 participant