ROCm / tensorflow-upstream

TensorFlow ROCm port
https://tensorflow.org
Apache License 2.0
688 stars 94 forks source link

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

Open ZappaBoy opened 1 year ago

ZappaBoy commented 1 year ago

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 commented 1 year ago

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

jayfurmanek commented 1 year ago

Thanks! I also have this PR https://github.com/ROCmSoftwarePlatform/tensorflow-upstream/pull/2171 that may help

jayfurmanek commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 (https://github.com/tensorflow/tensorflow/pull/61444#issuecomment-1662456968) ?. Maybe you can explain better than me due to your collaboration in this project.

ZappaBoy commented 1 year ago

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 (https://github.com/ROCmSoftwarePlatform/tensorflow-upstream/pull/2174) to fix that in case tensorflow main repository will not accept the other PR.

ZappaBoy commented 1 year ago

@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 (https://github.com/ROCmSoftwarePlatform/tensorflow-upstream/pull/2174).

ZappaBoy commented 1 year ago

@jayfurmanek any update on this issue/PR?