🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/140320

• 📄 Preview Python docs built from this PR
• 📄 Preview C++ docs built from this PR
• ❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit b5f6863 with merge base 8bdcdae ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorchbot label "topic: not user facing"

I left a more extended comment here #140318 (comment)

I'm not sure adding ROCR_VISIBLE_DEVICES to the _parse_visible_devices logic like this is a true fix here, I think this is a workaround. That being said there does appear to be issues with the current implementation more details in issue above. I think this may be due to us returning incorrect device count that no longer respects ROCR_VISIBLE_DEVICES

Added a new PR here #140398

Which will ensure interoperability between the two visible devices

Looks right. @tbennun can you please post some examples showing what the updated logic gives as output of _parse_visible_devices()?

@jithunnair-amd Of course, essentially now PyTorch takes ROCR_VISIBLE_DEVICES into account the same way it would with CUDA_VISIBLE_DEVICES.

@jataylo already gave some examples in #140318 (comment) so I can start from there.

Would you like this in the form of a test with, e.g., ROCR_VISIBLE_DEVICES=0,3,4 and HIP_VISIBLE_DEVICES=1,2?
I will use this opportunity to fix the mypy issue too.

@jithunnair-amd @jataylo please re-review. I added tests, fixed the linter issue, and improved the behavior when both environment variables are given. Thanks!

@jithunnair-amd @jataylo Any updates on this PR? Thanks!

LGTM, approved workflow to see if the UT passes.

@pytorchbot merge

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Merge failed

Reason: 1 jobs have failed, first few of them are: trunk / win-vs2019-cuda12.1-py3 / build

The failures seem unrelated to this PR and related to the version of distutils.

@pytorchbot rebase

Rebasing to see if unrelated errors go away :)

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchbot revert -m 'Sorry for reverting your change but test_hip_device_count is failing in trunk after this land' -c nosignal

test_cuda.py::TestCuda::test_hip_device_count GH job link HUD commit link

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@tbennun your PR has been successfully reverted.

@huydhn Thanks. The test passed locally, I will check.

Strange that it didn't show up in the tests. Are rocm tests not running on PRs? If not, is there a way I can trigger them?

It could be an issue with our target determination when some tests were wrongly skipped. Let me double check that, but you could addci-no-td on your PR to opt-out and this will ensure that all tests are run on your reland PR

It doesn't looks like the case, I think you just need to add ciflow/rocm to run all ROCm tests then. Having both ci-no-td and ciflow/rocm labels just to be sure will do.

Turns out the rocm tests did run (and terminated successfully) before: https://github.com/pytorch/pytorch/actions/runs/12199014152/job/34033407881#step:15:961

@tbennun I wonder if it has anything to do with the CI jobs setting HIP_VISIBLE_DEVICES=0 before execution then us trying to set ROCR_VISIBLE_DEVICES=0,1,2 at runtime during the UT.

The application may only see a single GPU and then try to set device_count to 3. Might need some local testing to try and reproduce.

@jataylo it might be, which is why I made sure to remove the environment variable, like other tests do, in the reland PR (#142292). What’s weird is that it passed on the PR build but not on main afterwards. Regardless, the new PR should resolve potential index errors.