rules_python icon indicating copy to clipboard operation
rules_python copied to clipboard

feat: add ability to override hub name

Open finn-ball opened this issue 10 months ago • 9 comments

Allows the user to override pip dependencies:

https://github.com/bazelbuild/rules_python/issues/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.

finn-ball avatar Mar 31 '24 14:03 finn-ball

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.

google-cla[bot] avatar Mar 31 '24 14:03 google-cla[bot]

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.

hofbi avatar Jul 25 '24 12:07 hofbi

Thanks for the message. Could you explain, what is the reason for using a particular version of the python dependencies for rules_ros? Is it security, other type of compliance, or just need to fix bad behaviour in rules_ros?

The bzlmod APIs may still need a little bit of refinement before releasing 1.0 and I would like to understand if the feature you'd like is just a workaround a different design flaw that you are experiencing. In order to ensure that we get the right primitives in place I would like to analyse each request separately.

On 25 July 2024 15:49:20 EEST, Markus Hofbauer @.***> wrote:

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.

-- Reply to this email directly or view it on GitHub: https://github.com/bazelbuild/rules_python/pull/1829#issuecomment-2250242560 You are receiving this because you commented.

Message ID: @.***>

aignas avatar Jul 25 '24 14:07 aignas

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 avatar Jul 25 '24 14:07 hofbi

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

hofbi avatar Jul 30 '24 12:07 hofbi

Thanks for bringing this back to the top of my inbox. Could you clarify why you would like to use the same version everywhere?

On 30 July 2024 15:41:26 EEST, Markus Hofbauer @.***> wrote:

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

-- Reply to this email directly or view it on GitHub: https://github.com/bazelbuild/rules_python/pull/1829#issuecomment-2258256315 You are receiving this because you were mentioned.

Message ID: @.***>

aignas avatar Jul 30 '24 13:07 aignas

So that we can follow https://opensource.google/documentation/reference/thirdparty/oneversion

hofbi avatar Jul 30 '24 14:07 hofbi

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.

finn-ball avatar Jul 31 '24 13:07 finn-ball

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 #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.

aignas avatar Sep 08 '24 12:09 aignas