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

cycles between torch 2.0.0 and triton 2.0.0 #1188

Closed
qzmfranklin opened this issue Apr 30, 2023 · 4 comments
Closed

cycles between torch 2.0.0 and triton 2.0.0 #1188

qzmfranklin opened this issue Apr 30, 2023 · 4 comments

Comments

@qzmfranklin
Copy link

Thanks a lot for all the great work put in rules_python.

I really enjoyed the multi-version and vendored toolchain support.

Ran into a cycled dependency issue, as follows:

bazel: 6.1.1 (though I don' this this is relevant)

rules_python: a recent HEAD (1e869d8)

repro:

WORKSPACE:

load("@rules_python//python:repositories.bzl", "python_register_multi_toolchains")
load("@rules_python//python/pip_install:repositories.bzl", "pip_install_dependencies")

pip_install_dependencies()
python_register_multi_toolchains(
    name = "python",
    default_version = "3.10",
    python_versions = [
        "3.8",
        "3.9",
        "3.10",
        "3.11",
    ],
    register_coverage_tool = True,
)

load("@python//:pip.bzl", "multi_pip_parse")
load("@python//3.10:defs.bzl", interpreter_3_10 = "interpreter")
load("@python//3.11:defs.bzl", interpreter_3_11 = "interpreter")
load("@python//3.8:defs.bzl", interpreter_3_8 = "interpreter")
load("@python//3.9:defs.bzl", interpreter_3_9 = "interpreter")

multi_pip_parse(
    name = "pypi",
    default_version = "3.10",
    python_interpreter_target = {
        "3.10": interpreter_3_10,
        "3.11": interpreter_3_11,
        "3.8": interpreter_3_8,
        "3.9": interpreter_3_9,
    },
    requirements_lock = {
        "3.10": "//third_party/python/common:requirements.lock.3_10.txt",
        "3.11": "//third_party/python/common:requirements.lock.3_11.txt",
        "3.8": "//third_party/python/common:requirements.lock.3_8.txt",
        "3.9": "//third_party/python/common:requirements.lock.3_9.txt",
    },
    extra_pip_args = [
        "--index-url",
        "https://pypi.tuna.tsinghua.edu.cn/simple",
        # "--no-dependencies",
    ],
)

requirements.in:

torch==2.0.0

Run

bazel build @pypi_3_8_torch//:pkg

yields the following error:

ERROR: /home/zhongmingqu/.cache/bazel/_bazel_zhongmingqu/9dac6aec585d38dc753dcfb84402be22/external/pypi_3_8_triton/BUILD.bazel:22:11: in py_library rule @pypi_3_8_triton//:pkg: cycle in dependency graph:
.-> @pypi_3_8_triton//:pkg (4204372a7bb0ec55b382995a9796bae6a21218e5607b1f1ed1019218df840129)
|   @pypi_3_8_torch//:pkg (4204372a7bb0ec55b382995a9796bae6a21218e5607b1f1ed1019218df840129)
`-- @pypi_3_8_triton//:pkg (4204372a7bb0ec55b382995a9796bae6a21218e5607b1f1ed1019218df840129)
WARNING: errors encountered while analyzing target '@pypi_3_8_triton//:pkg': it will not be built
ERROR: /home/zhongmingqu/.cache/bazel/_bazel_zhongmingqu/9dac6aec585d38dc753dcfb84402be22/external/pypi_3_8_triton/BUILD.bazel:22:11: in py_library rule @pypi_3_8_triton//:pkg: cycle in dependency graph: target depends on an already-reported cycle
WARNING: errors encountered while analyzing target '@pypi_3_8_triton//:pkg': it will not be built

A quick fix for me would be to remove the dependency of torch from triton. Afterall, triton only depends on torch for testing. It's optional.

I looked around and found package_annotation. But it does not allow removing an edge.

I also found that the wheel_installer.py already removed self-edge. But it does not remove cycles in the general case. Understandable, it's a wheel_installer, not a wheels_installer.

I think the fundamental issue is that wheel_installer includes all dependencies instead of just the core dependencies.

What would be a suggested fix from rules_python's authors for the situation I'm in?

@aignas
Copy link
Collaborator

aignas commented May 1, 2023

This is potentially a duplicate of #1076.

@chrislovecnm
Copy link
Collaborator

@qzmfranklin do we have a circular dependency? I know they are allowed in Python, but bazel does not, at this point, allow for them. I am thinking you may want to see if torch and triton are addressing this.

potiuk added a commit to potiuk/airflow that referenced this issue May 23, 2023
In the last wave of providers apache#31416 we bumped min-airlfow-version to 2.4 and
added mechanism to verify min-airflow version is ok while importing, but
it turned out that there are cases where installing just old version of
airflow (with no constraints) brings the latest version of those
providers and causes new installation of airflow to fail. This is far
too common to ignore or require to use constraints, unfortunately  We do
not have min-airlfow-version in the preinstalled providers for one
reason only. For some tools that are NOT conforming to standards (such
as bazel), having min-airflow-version for those providers causes
circular dependency (even though technically dependencies in PyPI can -
and often are - circular, because dependencies in Python are not DAG and
can contain cycles.

This was in response to this issue in 2021 apache#17795.

However, this is really  bazel issue, and on top of it - it's recognized
as such and being fixed very recently
in bazelbuild/rules_python#1188 because there
are other packages that have similar problems (pytorch and triton being
popular couple). Also Bazel is not that popular in the Python world. 

Therefore, rather than trying to workaround the problem of bazel, we
encourage them to merge and release the fix
bazelbuild/rules_python#1166 (comment)
and call it out in our installation instructions, that bazel
installation might lead to problems like that.

If bazel does not fix it, this will only be a problem for Future
installations of airflow in a few months most likely. It will not impact
current bazel users installing old versions of Airflow (actually they
might start having problems now if we do not fix it and yank the 5
providers released yesterday)
potiuk added a commit to apache/airflow that referenced this issue May 23, 2023
In the last wave of providers #31416 we bumped min-airlfow-version to 2.4 and
added mechanism to verify min-airflow version is ok while importing, but
it turned out that there are cases where installing just old version of
airflow (with no constraints) brings the latest version of those
providers and causes new installation of airflow to fail. This is far
too common to ignore or require to use constraints, unfortunately  We do
not have min-airlfow-version in the preinstalled providers for one
reason only. For some tools that are NOT conforming to standards (such
as bazel), having min-airflow-version for those providers causes
circular dependency (even though technically dependencies in PyPI can -
and often are - circular, because dependencies in Python are not DAG and
can contain cycles.

This was in response to this issue in 2021 #17795.

However, this is really  bazel issue, and on top of it - it's recognized
as such and being fixed very recently
in bazelbuild/rules_python#1188 because there
are other packages that have similar problems (pytorch and triton being
popular couple). Also Bazel is not that popular in the Python world. 

Therefore, rather than trying to workaround the problem of bazel, we
encourage them to merge and release the fix
bazelbuild/rules_python#1166 (comment)
and call it out in our installation instructions, that bazel
installation might lead to problems like that.

If bazel does not fix it, this will only be a problem for Future
installations of airflow in a few months most likely. It will not impact
current bazel users installing old versions of Airflow (actually they
might start having problems now if we do not fix it and yank the 5
providers released yesterday)
@ph03
Copy link

ph03 commented Jun 1, 2023

Had the same issue with pytorch2.0 / triton and resolved this by patching the triton wheel to remove the pytorch-dependency from it (resolving the circular dependency affecting the bazel-side)

ahidalgob pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this issue Jun 12, 2023
In the last wave of providers #31416 we bumped min-airlfow-version to 2.4 and
added mechanism to verify min-airflow version is ok while importing, but
it turned out that there are cases where installing just old version of
airflow (with no constraints) brings the latest version of those
providers and causes new installation of airflow to fail. This is far
too common to ignore or require to use constraints, unfortunately  We do
not have min-airlfow-version in the preinstalled providers for one
reason only. For some tools that are NOT conforming to standards (such
as bazel), having min-airflow-version for those providers causes
circular dependency (even though technically dependencies in PyPI can -
and often are - circular, because dependencies in Python are not DAG and
can contain cycles.

This was in response to this issue in 2021 #17795.

However, this is really  bazel issue, and on top of it - it's recognized
as such and being fixed very recently
in bazelbuild/rules_python#1188 because there
are other packages that have similar problems (pytorch and triton being
popular couple). Also Bazel is not that popular in the Python world. 

Therefore, rather than trying to workaround the problem of bazel, we
encourage them to merge and release the fix
bazelbuild/rules_python#1166 (comment)
and call it out in our installation instructions, that bazel
installation might lead to problems like that.

If bazel does not fix it, this will only be a problem for Future
installations of airflow in a few months most likely. It will not impact
current bazel users installing old versions of Airflow (actually they
might start having problems now if we do not fix it and yank the 5
providers released yesterday)

GitOrigin-RevId: 547e352578fac92f072b269dc257d21cdc279d97
@groodt
Copy link
Collaborator

groodt commented Jul 19, 2023

Closing this as a duplicate of #1076

For what it's worth, there are workarounds for the torch / triton situation that may be suitable for anyone on this issue: pytorch/pytorch#99622 (comment)

@groodt groodt closed this as completed Jul 19, 2023
github-merge-queue bot pushed a commit that referenced this issue Nov 28, 2023
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]>
ahidalgob pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this issue May 15, 2024
In the last wave of providers #31416 we bumped min-airlfow-version to 2.4 and
added mechanism to verify min-airflow version is ok while importing, but
it turned out that there are cases where installing just old version of
airflow (with no constraints) brings the latest version of those
providers and causes new installation of airflow to fail. This is far
too common to ignore or require to use constraints, unfortunately  We do
not have min-airlfow-version in the preinstalled providers for one
reason only. For some tools that are NOT conforming to standards (such
as bazel), having min-airflow-version for those providers causes
circular dependency (even though technically dependencies in PyPI can -
and often are - circular, because dependencies in Python are not DAG and
can contain cycles.

This was in response to this issue in 2021 #17795.

However, this is really  bazel issue, and on top of it - it's recognized
as such and being fixed very recently
in bazelbuild/rules_python#1188 because there
are other packages that have similar problems (pytorch and triton being
popular couple). Also Bazel is not that popular in the Python world. 

Therefore, rather than trying to workaround the problem of bazel, we
encourage them to merge and release the fix
bazelbuild/rules_python#1166 (comment)
and call it out in our installation instructions, that bazel
installation might lead to problems like that.

If bazel does not fix it, this will only be a problem for Future
installations of airflow in a few months most likely. It will not impact
current bazel users installing old versions of Airflow (actually they
might start having problems now if we do not fix it and yank the 5
providers released yesterday)

GitOrigin-RevId: 547e352578fac92f072b269dc257d21cdc279d97
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this issue Sep 19, 2024
In the last wave of providers #31416 we bumped min-airlfow-version to 2.4 and
added mechanism to verify min-airflow version is ok while importing, but
it turned out that there are cases where installing just old version of
airflow (with no constraints) brings the latest version of those
providers and causes new installation of airflow to fail. This is far
too common to ignore or require to use constraints, unfortunately  We do
not have min-airlfow-version in the preinstalled providers for one
reason only. For some tools that are NOT conforming to standards (such
as bazel), having min-airflow-version for those providers causes
circular dependency (even though technically dependencies in PyPI can -
and often are - circular, because dependencies in Python are not DAG and
can contain cycles.

This was in response to this issue in 2021 #17795.

However, this is really  bazel issue, and on top of it - it's recognized
as such and being fixed very recently
in bazelbuild/rules_python#1188 because there
are other packages that have similar problems (pytorch and triton being
popular couple). Also Bazel is not that popular in the Python world. 

Therefore, rather than trying to workaround the problem of bazel, we
encourage them to merge and release the fix
bazelbuild/rules_python#1166 (comment)
and call it out in our installation instructions, that bazel
installation might lead to problems like that.

If bazel does not fix it, this will only be a problem for Future
installations of airflow in a few months most likely. It will not impact
current bazel users installing old versions of Airflow (actually they
might start having problems now if we do not fix it and yank the 5
providers released yesterday)

GitOrigin-RevId: 547e352578fac92f072b269dc257d21cdc279d97
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

No branches or pull requests

5 participants