-
Notifications
You must be signed in to change notification settings - Fork 540
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
Handle cyclic dependency in pip wheels, e.g. PyTorch 2.0 #1076
Comments
Does Gazelle actually have access to this information? Consider a package with platform-dependent dependencies and packaging. Gazelle can't reasonably be made to determine that 1) there is a conditional dependency 2) on some platform+architecture combination it results in a cycle. The dependency graph is only really available after the Maybe one could make |
In my intermediate solution, I am not suggesting that Gazelle do anything. The only comment I made regarding Gazelle is that an extra resolve directive would be enough to make things work there without any changes to what it has currently. In fact, looking at how it works more closely, I think Gazelle will work for my suggestion without any changes to it. My suggestion is all this could be handled at the For PyTorch 2.0, one could imagine a new
With automatic cycle detection, the invocation would stay as-is. Going back to the more long-term proper solution, I was suggesting something akin to what Gazelle does for Golang, where dependencies are specified per Go package, and not per Go module. And Gazelle accounts for platform specific dependencies seamlessly there. It is able to do this by running Gazelle inside each |
At least |
That makes good sense. Even being able to manually specify composite libraries would be a big help. At present my deployment is maintaining some forked packages with "fixed" dependencies to resolve cycles that way. |
I've pulled together a draft patch which implements the above sketched |
I ran into the same problem with the same dependency (PyTorch 2.0). This is an important package, so I think we need to resolve this sooner rather than later. A few observations:
if whl.name == "triton":
sanitised_dependencies = [s for s in sanitised_dependencies if s != '"@pip_torch//:pkg"']
sanitised_wheel_file_dependencies = [s for s in sanitised_wheel_file_dependencies if s != '"@pip_torch//:whl"' The patch can be maintained locally by changing your rules_python At a higher level, I'd like to learn more about how the Python ecosystem uses cyclic dependencies, and how we should translate that into a bazel target graph. A quick grep through the triton source code shows that the back-dependency to torch is not easily removed; it's imported from many places. A couple questions for the OP @siddharthab:
Do you know if any PEP specifies what should happen in the presence of cyclic imports?
It sounds like you have an idea for how this might be done. Could you be more specific? When a My concern with the |
I'd argue that all of Re: my patchset, still chasing our lawyers. |
A point to be noted here is that we are talking about cyclic dedendencies in wheels and not python packages. I did not find something specifically about cyclic imports in wheels, but it is to be expected from the specification of file contents in a wheel. It simply states that the root contains all files to be installed in A note on circular Python package imports. While I could not find a PEP or reference doc talking about this specifically either, it is possible from the reference on the import mechanism, because an
Yes, I gave a little more thought to this since I posted the issue and I am leaning towards this solution as the best way forward. I also want to implement proper granularity in Gazelle's Python extension, for the benefit of test caching in our internal codebase. Please bear with me if I don't use the right terms or get something wrong because I am new to Python and its packaging system. As per the spec, there are no restrictions on what file/directory names may exist in a wheel (only
In my ideal scenario, a Bazel repository initialized from a Python wheel is "spread" out as defined in the spec, and then we run Gazelle to build a finer dependency graph. Gazelle's Python extension currently does not do some of these things correctly.
Short of running Gazelle inside an unpacked wheel (with or without my suggested changes), I think a composite wheel is the only way forward to keep everything functional (or you could just omit dependencies because you know you won't use that code). For example, if we just descended into the immediate top-level subdirectories inside a wheel and defined |
Edited the last paragraph on my previous comment. |
Cyclic dependencies are invalid / unsupported in the rest of the PyPA ecosystem as well. It’s just that pip has historically not enforced it. Poetry, conda and others are a bit stricter. The upstream issue with OpenAI/triton has been fixed triton-lang/triton#1374 |
In the issue you linked above, triton has moved torch to be a test dependency in its metadata file, but it still maintains imports to torch in its modules, and likewise, torch continues to maintain imports to triton in its modules. Because this is valid Python, this will continue to work as long as the user installs both torch and triton, manually installing the other if needed. This means that Bazel users will also have to manually specify the other dependency in their BUILD file. This solution just shifts the burden to the users, which may be OK as a special case here. I suppose there are some other packages which do the same. Thanks for looking into this everyone. I think I am OK with the solution (triton deleting the link in its metadata) for now. |
Is this being maintained or is this being ignored? It is clear that including both dependencies does not work and if this rules package is no longer being maintained what is the correct package? |
The idea here is that it will work once the torch and triton wheels have been updated to remove the cyclic dependency. I am not sure what the plan there is, we may have to wait a few more days. |
I see. I have found a solution in the mean time of running pip at runtime with
|
This issue should be reopened. The circular dependency is still present as of the latest PyTorch release (2.0.1), as I've documented upstream: triton-lang/triton#1374 (comment). The patch I described in #1076 (comment) above still works, but carrying such a patch for months is not a good long-term solution. I'm sympathetic to arguments that the Python ecosystem "shouldn't" support cyclic deps. But PyTorch is an important part of the Python ecosystem. rules_python should support it. |
I reopened this issue, but if I remember correctly, this is more a bazel problem and not a rules problem. |
This particular issue with torch and triton has a workaround: pytorch/pytorch#99622 (comment) |
@alexeagle, @rickeylev et al., I wanted to verify that that bazel the binary does not support circular dependencies, and we should raise the issue there. @groodt, do you mind summarizing how a user could work around this problem? It is something we should document instead of having a user dig through this issue. |
@chrislovecnm Yes, bazel certainly doesn't support cycles between targets. Here are the error messages that you will see:
Notice the This can be reported to bazelbuild/bazel upstream, but in general, I very much doubt that adding support for cycles is something that can be easily supported in bazel itself. There is some discussion about cycles in the docs, but given that bazel is fundamentally built on DAGs, I'm not really convinced it would be easy to add or even a good idea. In terms of docs, guidance and workarounds. Yes, we probably should add some user level docs for the questions and best-practices that we constantly get asked about. There are some discussions that start around in this PR: #1166 (comment) For this particular issue (and cycles in general), we can add |
Chiming in that this is now affecting the latest version of Sphinx, another widely-used package. I've got a minimal reproducing example and more details here: |
@bruno-digitbio |
I worked around the Torch/Triton problem by manually patching the Torch wheel. |
@georgevreilly very simple follow up, but how does one install the manually generated wheel? I have not been able to figure out how to expose a local copy of the wheel to Bazel. |
Update: I managed to solve this issue following the this comment as mentioned above. I have run into some CUDA dependency issues, but the minimal repo that I link below solves the issue if you replace https://github.com/OliverFM/pytorch_with_gazelle That said, I would still be very interested in seeing the last steps @georgevreilly's solution as I think that in some cases it could work a lot better. |
We do some extremely bespoke things with installing Python packages, and we're currently in the process of switching over to rules_py because it fixes the overlong We do use Artifactory to manage a private Python repository and the package can be installed from there with |
Before that the users had to rely on patching the actual wheel files and uploading them as different versions to internal artifact stores if they needed to modify the wheel dependencies. This is very common when breaking dependency cycles in `pytorch` or `apache-airflow` packages. With this feature we can support patching external PyPI dependencies via unified patches passed into the `pip.whl_mods` extension and the legacy `package_annotation` macro. Fixes bazelbuild#1076.
Before that the users had to rely on patching the actual wheel files and uploading them as different versions to internal artifact stores if they needed to modify the wheel dependencies. This is very common when breaking dependency cycles in `pytorch` or `apache-airflow` packages. With this feature we can support patching external PyPI dependencies via unified patches passed into the `pip.whl_mods` extension and the legacy `package_annotation` macro. Fixes bazelbuild#1076.
FYI I needed this to be able to use Airflow I have made #1393 which proposes a potential API for @groodt and @rickeylev, I reused the |
This is an awesome feature that will also become handy to resolve other issues that require fiddling with the wheel's contents (I had a couple of other issues that I had to address by updating the wheels themselves, and patching them would be so much easier 👍) |
X-posting from #1166 (comment) for increased visibility. |
Yes. Totally. Our fork at work has very basic patching functionality. It's much more pleasant and flexible solution and broadly is understandable by most people using bazel because it's quite a familiar API used for batching external repositories and http_archives etc. We've got a few different options in the works. I think some form of patching will land at some stage. For anyone looking for simple workarounds right now, this is a very easy option for you: #1166 (comment) |
Before that the users had to rely on patching the actual wheel files and uploading them as different versions to internal artifact stores if they needed to modify the wheel dependencies. This is very common when breaking dependency cycles in `pytorch` or `apache-airflow` packages. With this feature we can support patching external PyPI dependencies via unified patches passed into the `pip.whl_mods` extension and the legacy `package_annotation` macro. Fixes bazelbuild#1076.
Before that the users had to rely on patching the actual wheel files and uploading them as different versions to internal artifact stores if they needed to modify the wheel dependencies. This is very common when breaking dependency cycles in `pytorch` or `apache-airflow` packages. With this feature we can support patching external PyPI dependencies via unified patches passed into the `pip.whl_mods` extension and the legacy `package_annotation` macro. Fixes bazelbuild#1076. Add a non-empty patch and show that there can be multiple patches exp: A different design, that does not require us to put patches to annotations.json Simplify the design and add extra notes in the implementation fix: make the legacy WORKSPACE patching compatible with bazel 5.4 chore: update docs refactor: s/module_override/whl_override doc: update changelog doc: improve documentation and code comments on the new features s/whl_override/whl_library_override/g refactor wheel installer feat: support whl_overriding before extraction Add a note doc: update changelog fixup: set better default values for patches chore: add wheel_repackager.py to the list of pysrcs fix rebase conflicts fix: use better defaults for annotations fixup: update docs fixup: minor tidy up add a comment on annotation support for bzlmod refactor: whl patching to a separate function finish cleaning up handling of whl_patches for python annotations Add an empty patch to the pip_repository_annotations example Move patch argument processing to a single place, next to annotations Improve the script and add logging feat!: remove legacy bzlmod patching example doc: remove changelog entry for legacy patching feat!: remove the patching support via pip annotations feat!: remove support for whl_library patching for now
Before that the users had to rely on patching the actual wheel files and uploading them as different versions to internal artifact stores if they needed to modify the wheel dependencies. This is very common when breaking dependency cycles in `pytorch` or `apache-airflow` packages. With this feature we can support patching external PyPI dependencies via unified patches passed into the `pip.whl_mods` extension and the legacy `package_annotation` macro. Fixes bazelbuild#1076. Add a non-empty patch and show that there can be multiple patches exp: A different design, that does not require us to put patches to annotations.json Simplify the design and add extra notes in the implementation fix: make the legacy WORKSPACE patching compatible with bazel 5.4 chore: update docs refactor: s/module_override/whl_override doc: update changelog doc: improve documentation and code comments on the new features s/whl_override/whl_library_override/g refactor wheel installer feat: support whl_overriding before extraction Add a note doc: update changelog fixup: set better default values for patches chore: add wheel_repackager.py to the list of pysrcs fix rebase conflicts fix: use better defaults for annotations fixup: update docs fixup: minor tidy up add a comment on annotation support for bzlmod refactor: whl patching to a separate function finish cleaning up handling of whl_patches for python annotations Add an empty patch to the pip_repository_annotations example Move patch argument processing to a single place, next to annotations Improve the script and add logging feat!: remove legacy bzlmod patching example doc: remove changelog entry for legacy patching feat!: remove the patching support via pip annotations feat!: remove support for whl_library patching for now
Before that the users had to rely on patching the actual wheel files and uploading them as different versions to internal artifact stores if they needed to modify the wheel dependencies. This is very common when breaking dependency cycles in `pytorch` or `apache-airflow` packages. With this feature we can support patching external PyPI dependencies via unified patches passed into the `pip.whl_mods` extension and the legacy `package_annotation` macro. Fixes bazelbuild#1076. Add a non-empty patch and show that there can be multiple patches exp: A different design, that does not require us to put patches to annotations.json Simplify the design and add extra notes in the implementation fix: make the legacy WORKSPACE patching compatible with bazel 5.4 chore: update docs refactor: s/module_override/whl_override doc: update changelog doc: improve documentation and code comments on the new features s/whl_override/whl_library_override/g refactor wheel installer feat: support whl_overriding before extraction Add a note doc: update changelog fixup: set better default values for patches chore: add wheel_repackager.py to the list of pysrcs fix rebase conflicts fix: use better defaults for annotations fixup: update docs fixup: minor tidy up add a comment on annotation support for bzlmod refactor: whl patching to a separate function finish cleaning up handling of whl_patches for python annotations Add an empty patch to the pip_repository_annotations example Move patch argument processing to a single place, next to annotations Improve the script and add logging feat!: remove legacy bzlmod patching example doc: remove changelog entry for legacy patching feat!: remove the patching support via pip annotations feat!: remove support for whl_library patching for now
This class is for being able to more easily recreate a wheel file after extracting it. This is not intended for usage outside the rules_python project. Towards bazelbuild#1076
…parate executions (#1487) Before the PR the downloading/building of the wheel and the extraction would be done as a single step, which meant that for patching of the wheel to happen, we would need to do it within the python script. In order to have more flexibility in the approach, this PR splits the process to two separate invocations of the wheel_installer, which incidentally also helps in a case where the downloading of the wheel file can happen separately via http_file. Related issues #1076, #1357
This class is for being able to more easily recreate a wheel file after extracting it. This is not intended for usage outside the rules_python project. Towards bazelbuild#1076
… RECORD (#1488) This class is for being able to more easily recreate a wheel file after extracting it. This is not intended for usage outside the rules_python project. Also stop sorting the entries when writing a RECORD file making the order of the RECORD file to be the same as the order the files to the zip file are added. Towards #1076
Before that the users had to rely on patching the actual wheel files and uploading them as different versions to internal artifact stores if they needed to modify the wheel dependencies. This is very common when breaking dependency cycles in `pytorch` or `apache-airflow` packages. With this feature we can support patching external PyPI dependencies via pip.override tag class to fix package dependencies and/or a broken `RECORD` metadata file. Overall design: * Split the `whl_installer` CLI into two parts - downloading and extracting. Merged in #1487. * Add a starlark function which extracts the downloaded wheel applies patches and repackages a wheel (so that the extraction part works as before). * Add a `override` tag_class to the `pip` extension and allow users to pass patches to be applied to specific wheel files. * Only the root module is allowed to apply patches. This is to avoid far away modules modifying the code of other modules and conflicts between modules and their patches. Patches have to be in `unified-diff` format. Related #1076, #1166, #1120
Now that #1393 has landed, I'm going to close this issue because installation cycles can be patched out in userland. We'll keep monitoring the situation and see how frequently any packages beyond torch==2.0.0 have cycles and whether the patching solution is adequate. |
This patch reworks the `pip_repository` machinery to allow users to manually annotate groups of libraries which form packaging cycles in PyPi and must be simultaneously installed. The strategy here is to transform any dependencies `A` and `B` which have dependencies and are mutually dependent ```mermaid graph LR; A-->B; A-->D; A-->E; B-->A; B-->F; B-->G; ``` into a new "dependency group" `C` which has `A*` and `B*` as dependencies, defined as `A` and `B` less any direct dependencies which are members of the group. This is viable _for python_ because Python files just need to be emplaced into a runfiles directory for the interpreter. We don't actually have a true hard dependency between the build definition of `A` requiring the build product `B` be available which requires that the build product of `A` be available. ```mermaid graph LR C-->A*; A*-->D; A*-->E; C-->B*; B*-->F; B*-->G; ``` This gets us most of the way there, as a user can now safely write `requirement("A")` and we can provide them with `C`, which has the desired effect of pulling in `A`, `B` and their respective transitives. There is one remaining problem - a user writing `deps = [requirement("A"), requirement("B")]` will take a double direct dependency on `C`. So we need to insert a layer of indirection, generating `C_A` and `C_B` which serve only as unique aliases for `C` so that we can support the double dependency. Our final dependency graph then is as follows ```mermaid graph LR C_A-->C; C_B-->C; C-->A*; A*-->D; A*-->E; C-->B*; B*-->F; B*-->G; ``` Addresses #1076, #1188 ## To do - [x] Get rebased - [x] Get re-validated manually - [x] Buildifier - [x] Get CI happy - [x] Update documentation - [x] Update changelog --------- Co-authored-by: Ignas Anikevicius <[email protected]>
🚀 feature request
Relevant Rules
pip_repository
,whl_library
andpy_library
Description
rules_python
does dependency resolution at the level of a wheel and sets up the entire wheel as onepy_library
. This difference in the level of granularity here is why we can not handle cyclic dependencies in wheels. Some pip wheels have cyclic dependencies which is valid Python, and pip handles them well. For example, the upcoming PyTorch 2.0 release has a cyclic dependency between thetorch
(.whl file) andpytorch-triton
(.whl file) wheels.Describe the solution you'd like
The proper solution for this would be to have more granular build units which would also be better from a sandboxing and build caching efficiency perspective. But as an intermediate solution, maybe we could install the wheels with cyclical dependencies between them into one repo (through one
whl_library
call) and set up the composite of such wheels as onepy_library
. For gazelle, a resolve directive can ensure that all packages point to the samepy_library
target.Describe alternatives you've considered
An alternative could be that I manually build a composite wheel and set it up as a
whl_library
in my WORKSPACE.The text was updated successfully, but these errors were encountered: