[ROCm] Updating ROCM_HOME handling for >ROCm 4.0 #55968

pruthvistony · 2021-04-13T22:49:28Z

This change is required to handle the case when hipcc is
updated to the latest using update-alternatives.
Update-alternatives support for few ROCm binaries is available
from ROCm 4.1 onwards.
This change doesnt not affect any previous versions of ROCm.

facebook-github-bot · 2021-04-13T22:49:34Z

💊 CI failures summary and remediations

As of commit 652f662 (more details on the Dr. CI page):

1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

pytorch_linux_xenial_cuda10_2_cudnn7_py3_gcc7_test2 (1/1)

Step: "Run tests" (full log | diagnosis details | 🔁 rerun)

Apr 14 18:22:53 AssertionError: False is not true : Scalars failed to compare as equal! Comparing 1 and 6 gives a difference of 5, but the allowed difference with rtol=0 and atol=0 is only 0!

Apr 14 18:22:53   test_simple_backward_same_input (__main__.TestMultithreadAutograd) ... ok (0.010s)
Apr 14 18:22:53 
Apr 14 18:22:53 ======================================================================
Apr 14 18:22:53 FAIL [0.003s]: test_record_function (__main__.TestAutograd)
Apr 14 18:22:53 ----------------------------------------------------------------------
Apr 14 18:22:53 Traceback (most recent call last):
Apr 14 18:22:53   File "test_autograd.py", line 3300, in test_record_function
Apr 14 18:22:53     self.assertEqual(idx, len(important_events))
Apr 14 18:22:53   File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 1371, in assertEqual
Apr 14 18:22:53     super().assertTrue(result, msg=self._get_assert_msg(msg, debug_msg=debug_msg))
Apr 14 18:22:53 AssertionError: False is not true : Scalars failed to compare as equal! Comparing 1 and 6 gives a difference of 5, but the allowed difference with rtol=0 and atol=0 is only 0!
Apr 14 18:22:53 
Apr 14 18:22:53 ----------------------------------------------------------------------
Apr 14 18:22:53 Ran 1105 tests in 660.612s
Apr 14 18:22:53 
Apr 14 18:22:53 FAILED (failures=1, skipped=5, expected failures=1)
Apr 14 18:22:53 
Apr 14 18:22:53 Generating XML reports...
Apr 14 18:22:53 Generated XML report: test-reports/python-unittest/test_autograd/TEST-TestAutograd-20210414181152.xml
Apr 14 18:22:53 Generated XML report: test-reports/python-unittest/test_autograd/TEST-TestAutogradComplex-20210414181152.xml
Apr 14 18:22:53 Generated XML report: test-reports/python-unittest/test_autograd/TEST-TestAutogradDeviceTypeCUDA-20210414181152.xml

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

pruthvistony · 2021-04-13T22:50:08Z

@jeffdaily @jithunnair-amd @sunway513
Please review this change.

ezyang · 2021-04-14T00:30:53Z

torch/utils/cpp_extension.py

nit: need a space after comma

ezyang · 2021-04-14T01:34:30Z

I'm not the ROCm expert but I don't like this patch. You can't assume just because hipcc is in usr that you're on a linux distribution with update-alternatives.

Conventionally, if there is a directory associated with a compiler, there is some command line flag you can pass to the compiler to get the paths. For example, you can do gcc -print-search-dirs to get the install dir. Does hipcc have something similar?

codecov · 2021-04-14T02:50:53Z

Codecov Report

Merging #55968 (22d09b2) into master (bbdb37b) will decrease coverage by 0.35%.
The diff coverage is 0.00%.

❗ Current head 22d09b2 differs from pull request most recent head 652f662. Consider uploading reports for the commit 652f662 to get more accurate results

@@            Coverage Diff             @@
##           master   #55968      +/-   ##
==========================================
- Coverage   77.14%   76.78%   -0.36%     
==========================================
  Files        1909     1909              
  Lines      189104   189110       +6     
==========================================
- Hits       145878   145215     -663     
- Misses      43226    43895     +669

jeffdaily · 2021-04-14T16:28:23Z

I'm not the ROCm expert but I don't like this patch. You can't assume just because hipcc is in usr that you're on a linux distribution with update-alternatives.

We discussed this and decided to use readlink -f to resolve ROCM_HOME. Updates from @pruthvistony to follow.

- This change is required to handle the case when hipcc is updated to the latest using update-alternatives. - This change doesnt not affect any previous versions of ROCm.

pruthvistony · 2021-04-14T16:58:03Z

I'm not the ROCm expert but I don't like this patch. You can't assume just because hipcc is in usr that you're on a linux distribution with update-alternatives.

We discussed this and decided to use readlink -f to resolve ROCM_HOME. Updates from @pruthvistony to follow.

As discussed I have updated the patch.
Please review.

pruthvistony · 2021-04-14T17:01:26Z

I'm not the ROCm expert but I don't like this patch. You can't assume just because hipcc is in usr that you're on a linux distribution with update-alternatives.

Conventionally, if there is a directory associated with a compiler, there is some command line flag you can pass to the compiler to get the paths. For example, you can do gcc -print-search-dirs to get the install dir. Does hipcc have something similar?

Currently in hipcc we dont have any options to get the installed dirs. I have updated the commit to NOT check the 'usr' directory and use readlink to get the final binary path, this works also when update-alternatives is used.

jeffdaily

LGTM if CI passes.

pruthvistony · 2021-04-14T23:37:47Z

The CI failure, doesnt seems to be related to this change

test_record_function - TestAutograd
test_autograd.py


Traceback (most recent call last):
  File "test_autograd.py", line 3300, in test_record_function
    self.assertEqual(idx, len(important_events))
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 1371, in assertEqual
    super().assertTrue(result, msg=self._get_assert_msg(msg, debug_msg=debug_msg))
AssertionError: False is not true : Scalars failed to compare as equal! Comparing 1 and 6 gives a difference of 5, but the allowed difference with rtol=0 and atol=0 is only 0!

facebook-github-bot · 2021-04-15T01:48:58Z

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

jithunnair-amd

LGTM

facebook-github-bot · 2021-04-15T14:50:16Z

@ezyang merged this pull request in b383b63.

Summary: - This change is required to handle the case when hipcc is updated to the latest using update-alternatives. - Update-alternatives support for few ROCm binaries is available from ROCm 4.1 onwards. - This change doesnt not affect any previous versions of ROCm. Pull Request resolved: pytorch#55968 Reviewed By: mruberry Differential Revision: D27785123 Pulled By: ezyang fbshipit-source-id: 8467e468d8d51277fab9b0c8cbd57e80bbcfc7f7

pruthvistony requested review from ezyang, fmassa and soumith as code owners April 13, 2021 22:49

facebook-github-bot added the cla signed label Apr 13, 2021

github-actions bot added the module: rocm AMD GPU support for Pytorch label Apr 13, 2021

pruthvistony force-pushed the rocm_41_update_alt_hipcc branch from 319f4d3 to 8f64472 Compare April 13, 2021 22:54

pytorchbot added the open source label Apr 13, 2021

pruthvistony force-pushed the rocm_41_update_alt_hipcc branch from 8f64472 to 22d09b2 Compare April 13, 2021 23:11

ezyang reviewed Apr 14, 2021

View reviewed changes

torch/utils/cpp_extension.py Outdated

Copy link

Contributor

ezyang Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: need a space after comma

jbschlosser added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Apr 14, 2021

[ROCm] Updating ROCM_HOME handling for >ROCm 4.0

652f662

- This change is required to handle the case when hipcc is updated to the latest using update-alternatives. - This change doesnt not affect any previous versions of ROCm.

pruthvistony force-pushed the rocm_41_update_alt_hipcc branch from 22d09b2 to 652f662 Compare April 14, 2021 16:56

pruthvistony requested a review from ezyang April 14, 2021 22:50

jeffdaily approved these changes Apr 14, 2021

View reviewed changes

ezyang approved these changes Apr 15, 2021

View reviewed changes

jithunnair-amd approved these changes Apr 15, 2021

View reviewed changes

facebook-github-bot closed this in b383b63 Apr 15, 2021

facebook-github-bot added the Merged label Apr 15, 2021

[ROCm] Updating ROCM_HOME handling for >ROCm 4.0 #55968

[ROCm] Updating ROCM_HOME handling for >ROCm 4.0 #55968

Uh oh!

Conversation

pruthvistony commented Apr 13, 2021

Uh oh!

facebook-github-bot commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

🕵️ 1 new failure recognized by patterns

pytorch_linux_xenial_cuda10_2_cudnn7_py3_gcc7_test2 (1/1)

Uh oh!

pruthvistony commented Apr 13, 2021

Uh oh!

ezyang Apr 14, 2021

Choose a reason for hiding this comment

Uh oh!

ezyang commented Apr 14, 2021

Uh oh!

codecov bot commented Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jeffdaily commented Apr 14, 2021

Uh oh!

pruthvistony commented Apr 14, 2021

Uh oh!

pruthvistony commented Apr 14, 2021

Uh oh!

jeffdaily left a comment

Choose a reason for hiding this comment

Uh oh!

pruthvistony commented Apr 14, 2021

Uh oh!

facebook-github-bot commented Apr 15, 2021

Uh oh!

jithunnair-amd left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Apr 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

facebook-github-bot commented Apr 13, 2021 •

edited

Loading

codecov bot commented Apr 14, 2021 •

edited

Loading