feat: support per-wheel enable_implicit_namespace_pkgs in whl_mods (WIP) #3279
Conversation
chrisirhc
requested review from
rickeylev,
aignas and
groodt
as code owners
Summary of ChangesHello @chrisirhc, 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 introduces a significant feature to 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
|
![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 introduces a valuable feature to control enable_implicit_namespace_pkgs on a per-wheel basis. The implementation is mostly correct, but there's a significant issue with using attr.bool for what should be a tri-state option in the bzlmod extension. This will cause the feature to not work as intended, as it will always override the global setting. My review includes suggestions to fix this by using attr.string instead, along with necessary adjustments in the consuming code and tests. I've also included a minor suggestion to improve a docstring for clarity.
| "enable_implicit_namespace_pkgs": attr.bool( | ||
| doc = """\ | ||
| (bool, optional): Override the global setting for generating __init__.py | ||
| files for namespace packages. If None, uses the repository-level setting. | ||
| """, | ||
| ), |
There was a problem hiding this comment.
Using attr.bool here will not work as intended. The goal is to have a tri-state value (True, False, or not set to use the global default), but attr.bool only supports True and False. If a user does not specify this attribute, it will default to False, incorrectly overriding the global setting.
To correctly implement the tri-state logic, I suggest using attr.string with a set of allowed values, for example "True", "False", and "auto" (for the default when not set).
You will also need to update the code that consumes this attribute to handle string values, which I've commented on in python/private/pypi/generate_whl_library_build_bazel.bzl.
"enable_implicit_namespace_pkgs": attr.string(
doc = """\
(string, optional): Override the global setting for generating __init__.py
files for namespace packages. Can be "True", "False", or "auto".
If "auto", uses the repository-level setting.
""",
default = "auto",
values = ["True", "False", "auto"],
),
| if hasattr(annotation, "enable_implicit_namespace_pkgs") and annotation.enable_implicit_namespace_pkgs != None: | ||
| kwargs["enable_implicit_namespace_pkgs"] = annotation.enable_implicit_namespace_pkgs |
There was a problem hiding this comment.
To accommodate the change from attr.bool to attr.string for enable_implicit_namespace_pkgs in whl_mods, and to correctly handle values from package_annotation (which can be boolean or None), this logic needs to be updated.
The new logic should check for None (from package_annotation), "auto" (the new default for whl_mods), and then convert string "True"/"False" or boolean True/False to the final boolean value that whl_library_targets expects.
| if hasattr(annotation, "enable_implicit_namespace_pkgs") and annotation.enable_implicit_namespace_pkgs != None: | |
| kwargs["enable_implicit_namespace_pkgs"] = annotation.enable_implicit_namespace_pkgs | |
| val = getattr(annotation, "enable_implicit_namespace_pkgs", None) | |
| if val == True or val == "True": | |
| kwargs["enable_implicit_namespace_pkgs"] = True | |
| elif val == False or val == "False": | |
| kwargs["enable_implicit_namespace_pkgs"] = False |
| enable_implicit_namespace_pkgs (bool, optional): Override the global setting for generating __init__.py | ||
| files for namespace packages. If None, uses the repository-level setting. |
There was a problem hiding this comment.
The type hint (bool, optional) is slightly misleading as the function also accepts None to use the repository-level setting. It would be clearer to specify that None is a valid type.
| enable_implicit_namespace_pkgs (bool, optional): Override the global setting for generating __init__.py | |
| files for namespace packages. If None, uses the repository-level setting. | |
| enable_implicit_namespace_pkgs (bool | None, optional): Override the global setting for generating __init__.py | |
| files for namespace packages. If None, uses the repository-level setting. |
| def _whl_mod(*, hub_name, whl_name, additive_build_content = None, additive_build_content_file = None, copy_executables = {}, copy_files = {}, data = [], data_exclude_glob = [], srcs_exclude_glob = [], enable_implicit_namespace_pkgs = None): | ||
| return struct( | ||
| hub_name = hub_name, | ||
| whl_name = whl_name, | ||
| additive_build_content = additive_build_content, | ||
| additive_build_content_file = additive_build_content_file, | ||
| copy_executables = copy_executables, | ||
| copy_files = copy_files, | ||
| data = data, | ||
| data_exclude_glob = data_exclude_glob, | ||
| srcs_exclude_glob = srcs_exclude_glob, | ||
| enable_implicit_namespace_pkgs = enable_implicit_namespace_pkgs, | ||
| ) |
There was a problem hiding this comment.
To align with the suggested change of using attr.string for enable_implicit_namespace_pkgs, this test helper should be updated to use string values. The default should be "auto".
| def _whl_mod(*, hub_name, whl_name, additive_build_content = None, additive_build_content_file = None, copy_executables = {}, copy_files = {}, data = [], data_exclude_glob = [], srcs_exclude_glob = [], enable_implicit_namespace_pkgs = None): | |
| return struct( | |
| hub_name = hub_name, | |
| whl_name = whl_name, | |
| additive_build_content = additive_build_content, | |
| additive_build_content_file = additive_build_content_file, | |
| copy_executables = copy_executables, | |
| copy_files = copy_files, | |
| data = data, | |
| data_exclude_glob = data_exclude_glob, | |
| srcs_exclude_glob = srcs_exclude_glob, | |
| enable_implicit_namespace_pkgs = enable_implicit_namespace_pkgs, | |
| ) | |
| def _whl_mod(*, hub_name, whl_name, additive_build_content = None, additive_build_content_file = None, copy_executables = {}, copy_files = {}, data = [], data_exclude_glob = [], srcs_exclude_glob = [], enable_implicit_namespace_pkgs = "auto"): | |
| return struct( | |
| hub_name = hub_name, | |
| whl_name = whl_name, | |
| additive_build_content = additive_build_content, | |
| additive_build_content_file = additive_build_content_file, | |
| copy_executables = copy_executables, | |
| copy_files = copy_files, | |
| data = data, | |
| data_exclude_glob = data_exclude_glob, | |
| srcs_exclude_glob = srcs_exclude_glob, | |
| enable_implicit_namespace_pkgs = enable_implicit_namespace_pkgs, | |
| ) |
| hub_name = "pypi", | ||
| whl_name = "simple", | ||
| additive_build_content = "# Custom build content", | ||
| enable_implicit_namespace_pkgs = True, |
There was a problem hiding this comment.
To align with the suggested change of using attr.string for enable_implicit_namespace_pkgs, this should be a string.
| enable_implicit_namespace_pkgs = True, | |
| enable_implicit_namespace_pkgs = "True", |
| data = [], | ||
| data_exclude_glob = [], | ||
| srcs_exclude_glob = [], | ||
| enable_implicit_namespace_pkgs = True, |
There was a problem hiding this comment.
The parse_modules function passes the attribute value through without conversion. With the suggested change to attr.string, this assertion should check for a string value.
| enable_implicit_namespace_pkgs = True, | |
| enable_implicit_namespace_pkgs = "True", |
Update: I was reading a thread in
#pythonand saw that this set to True when setting--incompatible_default_to_explicit_init_pyis actually desirable and forward-looking.So perhaps this PR is unnecessary for my use case, unless somebody runs into issues and needs to toggle it for some packages/wheels.
Other note: perhaps the
enable_implicit_namespace_pkgsdocumentation can be further clarified.As part of investigating aspect-build/rules_py#611 which requires
enable_implicit_namespace_pkgs = Trueas a workaround to unblock PEX creation, I looked into how to only toggle on this flag per wheel, as per aspect-build/rules_py#611 (comment) .The goal of this is to reduce reliance on this flag where it's not necessary.
This PR is a proof-of-concept to gather if there's any interest in this feature. The commits aren't cleaned up yet.
Not yet done: