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

feat: add ability to override hub name #1829

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

finn-ball
Copy link

Allows the user to override pip dependencies:

#1791

This primarily helps solve the issue whereby if your python project A depends on python project B and python project C such that A -> B, A -> C, we can now create pip files which resolve for all three repositories. Currently the dependencies in either B or C will override pip dependencies in A.

Copy link

google-cla bot commented Mar 31, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@finn-ball finn-ball force-pushed the finn/override-hub-name branch 2 times, most recently from 59f00a6 to fac08a7 Compare March 31, 2024 15:05
@@ -421,6 +427,12 @@ A dict of labels to wheel names that is typically generated by the whl_modificat
The labels are JSON config files describing the modifications.
""",
),
"override": attr.bool(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally think this is OK since this works similarly to how archive overrides work, but I would like @rickeylev and @groodt to also weigh in here.

This is an obvious case where we may want to have an escape hatch and because it is a root module, we can do this.

However, I am wondering about what happens if a module like this gets published.

@hofbi
Copy link

hofbi commented Jul 25, 2024

I think I ran exactly into this issue when I want to consume rules_ros which defines this hub. But I want to override the pip deps with the onces we define to have a single version used for all dependencies.

@aignas
Copy link
Collaborator

aignas commented Jul 25, 2024 via email

@hofbi
Copy link

hofbi commented Jul 25, 2024

The reason for overriding the versions of the Python dependencies is that we want to have the same version for all targets. If we don't do that, we could end up in a target that uses the same python package at 2 different versions

@hofbi
Copy link

hofbi commented Jul 30, 2024

@aignas Is there any information that we can provide to help you clarifying this?

@aignas
Copy link
Collaborator

aignas commented Jul 30, 2024 via email

@hofbi
Copy link

hofbi commented Jul 30, 2024

@finn-ball
Copy link
Author

It would match what WORKSPACE files do in that if you defined the python libraries before the third party dependency, you can override your thirdparty dependency's python libraries.

@aignas
Copy link
Collaborator

aignas commented Sep 8, 2024

Coming back to this after thinking about how the override API is going to
work for the python bzlmod extension (see PRs attached to #2081 if anyone
wants to follow along).

I am going to write some thoughts here to explore the problem a little bit more
so that I am sure that I am understanding the implications of the change
correctly. The alternative semantics of the APIs that we can implement here
are either merging overrides or complete override and ignoring hub repo defs
from non-root modules.

Merging the overrides with the initial definition. Since the initial definition
will be done in a non-root module, this makes the implementation difficult -
module_ctx.modules[0] will always be the root module and then what definition
takes precedence may be difficult to debug. We could definitely do this, but we
would probably need to have a separate tag class here.

Supporting only overriding - the root module user will have to specify all of
the parameters them selves. Since the root module comes first in the processing
order, we'll just have to silence the error that would be thrown. What we need
to do is to store somewhere that the hub_repo has been defined by the root
module and continue.

I am not fully sure if the current implementation does not have a bug around
this, so it would be great to have added unit tests, similar to how #2204 has
added them. I think that way we can correctly identify edge cases in the
overrides and ensure that we are handling correctly them. If I am reading the
code correctly, the current implementation might work only if the
pip.parse(override = True) is done with a python_version that is not the
same as the one from the non-root module.

What is more, I don't think that storing a single pip_attr and a list of
pip_attr.python_version is sound - the separate invocations in general may
have different attribute values and we should store something like:

    python_attrs = {python_attr.python_version: pip_attr}

To sum up, I think I am happy to merge an implementation that has:

  • Unit tests similar to how test(bzlmod): add python.toolchain unit tests #2204 does them - maybe extract a function that
    returns a pip_hub_map and then unit test that. We should definitely add a
    test ensuring that overriding for multiple python versions works.
  • Ignores non-root module definitions of hub repos with the same name. The root
    module will be responsible for all of the overrides.
  • Has extra docs on overrides explaining the API semantics.

Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

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

Needs further changes

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.

3 participants