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

rocm version via index url #313

Open
wants to merge 1 commit into
base: raw-png
Choose a base branch
from

Conversation

HPPinata
Copy link

@HPPinata HPPinata commented Oct 6, 2024

--extra-index-url https://download.pytorch.org/whl/rocmX.Y is sufficient to install the right package.
This also makes A/B version testing easier.

--extra-index-url https://download.pytorch.org/whl/rocmX.Y is sufficient to install the right package.
This also makes A/B version testing easier.
@tazlin
Copy link
Member

tazlin commented Oct 6, 2024

You did bring to my attention that the files did not match. However, you could practically get the same convenience that I think you're trying to bring to this file by using the main requirements.txt instead. It is supposed to be otherwise identical and I foresee that remaining to be the case.

The purpose of the rocm tag in the torch requirement is more to help avoid the pitfall of accidently installing the cpu or CUDA version of torch when using AMD. In fact, the existence of the requirements.rocm.txt file dates back to a time where you would have had to have had installed it manually via the command line.

There might be something I'm missing here. If you still think its worth adding, let me know why.

@HPPinata
Copy link
Author

HPPinata commented Oct 6, 2024

I just stumbled across this when trying to A/B test different ROCm versions.

With the explicit version tag removed this can be done exclusively in the Dockerfile building the environment.
If it's part of the requirements.txt I have to point the build to my fork.

I haven't looked at whether it "just works" with the standard requirements.txt but will test that shortly.

This isn't really too relevant (the fork exists now, might as well use it), just a bit annoying and a potentially unified requirements.txt might be a cleaner solution if it actually ends up working out.

@tazlin
Copy link
Member

tazlin commented Oct 6, 2024

Yea, as it is, the rocm reqs file is more of a legacy artifact from when support was experimental. Maybe it should be unified sooner than later.

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

Successfully merging this pull request may close these issues.

2 participants