fix(pip): Pass --abi flag when downloading wheel for freethreaded python#3762
fix(pip): Pass --abi flag when downloading wheel for freethreaded python#3762meteorcloudy wants to merge 1 commit intobazel-contrib:mainfrom
Conversation
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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).
| 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 |
| minor, _, _ = tail.partition(".") | ||
| if major and minor: | ||
| plat_args.append("--abi=cp{}{}t".format(major, minor)) | ||
| options[plat] = plat_args |
There was a problem hiding this comment.
With 2.0.0 and above this might not be needed, because by default we are using the downloader to fetch the wheels and the freethreaded wheels should be supported by default.
When
pip downloadis used for fetching pip dependency, we need to pass--abiflag 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.