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

FAKE_REQUIRED_PACKAGES tensorflow-intel prevent poetry installation of tensorflow-rocm #2161

Open
ZappaBoy opened this issue Jul 18, 2023 · 9 comments

Comments

@ZappaBoy
Copy link

ZappaBoy commented Jul 18, 2023

Issue type

Build/Install

Have you reproduced the bug with TensorFlow Nightly?

Yes

Source

source

TensorFlow version

2.12.0.560

Custom code

Yes

OS platform and distribution

Archlinux 6.1.38-2-lts

Mobile device

No response

Python version

3.11

Bazel version

No response

GCC/compiler version

No response

CUDA/cuDNN version

No response

GPU model and memory

AMD Radeon RX 6700 XT - gfx1031

Current behavior?

Updating tensorflow-rocm using poetry produce a not resolvable dependency error due to tensorflow-intel "fake required package".
The problem is due to the following lines:
https://github.com/ROCmSoftwarePlatform/tensorflow-upstream/blob/2612672170a29a8f6570e6c2dfdfa3d506bf2da8/tensorflow/tools/pip_package/setup.py#L168
https://github.com/ROCmSoftwarePlatform/tensorflow-upstream/blob/2612672170a29a8f6570e6c2dfdfa3d506bf2da8/tensorflow/tools/pip_package/setup.py#L171

In particular, the error is caused by the _VERSION. In fact, the same version of tensorflow-rocm (2.12.0.560) not exists in tensorflow-intel.

Standalone code to reproduce the issue

poetry new fixme
cd fixme
poetry add tensorflow-rocm=="2.12.0.560"

Relevant log output

Because tensorflow-rocm (2.12.0.560) depends on tensorflow-intel (2.12.0.560) which doesn't match any versions, tensorflow-rocm is forbidden.
So, because lstm-predictor depends on tensorflow-rocm (2.12.0.560), version solving failed.

Possible fix

Simply truncate the version to the patch version before the FAKE_REQUIRED_PACKAGES list definition.

# _VERSION="2.12.0.560"
_VERSION = (".").join(_VERSION.split(".")[:3])
@ZappaBoy ZappaBoy changed the title Tensorflow-intel FAKE_REQUIRED_PACKAGES prevent poetry installation FAKE_REQUIRED_PACKAGES tensorflow-intel prevent poetry installation of tensorflow-rocm Jul 18, 2023
@ZappaBoy ZappaBoy changed the title FAKE_REQUIRED_PACKAGES tensorflow-intel prevent poetry installation of tensorflow-rocm FAKE_REQUIRED_PACKAGES tensorflow-intel prevent poetry installation of tensorflow-rocm Jul 31, 2023
@ZappaBoy
Copy link
Author

ZappaBoy commented Aug 2, 2023

Hi all,
I created a PR on the main tensorflow repository (tensorflow#61444) .
At the moment the PR is in a review status but I think that this is a way to fix this issue.

@jayfurmanek
Copy link

Thanks! I also have this PR #2171 that may help

@jayfurmanek
Copy link

Actually can you check to see if my PR will help with this poetry issue? Looking at it again it may not. It does fix the nightly build issue we were seeing though, but perhaps we need something else for this issue.

@ZappaBoy
Copy link
Author

ZappaBoy commented Aug 2, 2023

Correct, unfortunately your PR doesn't fix this issue. The issue is strictly related to the FAKE_REQUIRED_PACKAGES and collaborator build REQUIRED_PACKAGES version that need to be truncated to the patch version level.
You can add this behavior when the update_version.py script is used with the --rocm_version argument, but in my honest opinion this is a partial fix of the issue because it affects only rocm while the changes I proposed will be useful for all actual and future forks. If mine changes will not accepted I think that you can add this feature but this means that all the tensorflow forks need to be the same creating their own argument of the update_version.py script with the same behavior that, as you can imagine, might be a little bit annoying.

@jayfurmanek
Copy link

yup, ok.
Can you open a PR to https://github.com/ROCmSoftwarePlatform/tensorflow-upstream with your change?
We can merge it there at least.

thx

@ZappaBoy
Copy link
Author

ZappaBoy commented Aug 2, 2023

yup, ok. Can you open a PR to https://github.com/ROCmSoftwarePlatform/tensorflow-upstream with your change? We can merge it there at least.

thx

Sure, meanwhile can you help me to explain to the reviewer how the setup.py is used in tensorflow-rocm (tensorflow#61444 (comment)) ?. Maybe you can explain better than me due to your collaboration in this project.

@ZappaBoy
Copy link
Author

ZappaBoy commented Aug 2, 2023

yup, ok. Can you open a PR to https://github.com/ROCmSoftwarePlatform/tensorflow-upstream with your change? We can merge it there at least.

thx

@jayfurmanek As you can see I opened a PR (#2174) to fix that in case tensorflow main repository will not accept the other PR.

@ZappaBoy
Copy link
Author

ZappaBoy commented Aug 2, 2023

@jayfurmanek the PR was rejected due to according to the reviewer this is an issue of tensorflow-rocm.

The reviewer suggest to change the following code in the setup.py:
https://github.com/ROCmSoftwarePlatform/tensorflow-upstream/blob/ec6152a25de72ad2a2384f14323713e04583f27c/tensorflow/tools/pip_package/setup.py#L193-L194

But, in my honest opinion I think that my changes still work without change the actual rocm _VERSION.
If not, let me know in the PR (#2174).

@ZappaBoy
Copy link
Author

@jayfurmanek any update on this issue/PR?

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

2 participants