Skip to content

Conversation

@skyline75489
Copy link
Contributor

@skyline75489 skyline75489 commented Nov 17, 2020

Inspired by #47993, this fixes the import error in collect_env.py with older version of PyTorch when torch.version does not have hip property.

The way to reproduce the crash is simple:

  1. Install torch==1.2.0.
  2. Run the collect_env.py

@dr-ci
Copy link

dr-ci bot commented Nov 17, 2020

💊 CI failures summary and remediations

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


  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

codecov.io: 1 failed


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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 5 times.

@codecov
Copy link

codecov bot commented Nov 17, 2020

Codecov Report

Merging #48076 (fc1ab5b) into master (af37f8f) will decrease coverage by 0.15%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master   #48076      +/-   ##
==========================================
- Coverage   81.31%   81.16%   -0.16%     
==========================================
  Files        1839     1839              
  Lines      198670   198670              
==========================================
- Hits       161544   161245     -299     
- Misses      37126    37425     +299     

@gchanan gchanan requested a review from malfet November 17, 2020 15:48
@gchanan gchanan added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Nov 17, 2020
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@samestep samestep left a comment

Choose a reason for hiding this comment

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

lgtm 👍 could you add a test plan to this PR though? I see a bunch of discussion in the linked issue, but it would be nice if you could summarize here the exact steps one would need to take to reproduce the past and current behavior.

@skyline75489
Copy link
Contributor Author

@samstep Hi. I updated the PR description about how to reproduce the crash. It's dead simple. I assume torch<=1.2.0 would all suffer from this. With this fix, it will not crash any more.

However I don't know what exactly a "test plan" is. Is it something I should add in the PR description, or is it something more like a unit test?

@samestep
Copy link
Contributor

@skyline75489 Awesome, thanks :) and no you don't need to add a unit test or anything, that's all I was looking for. Now I just need another FB employee to approve the diff that I imported so I can land your change 👍

@facebook-github-bot
Copy link
Contributor

@samestep merged this pull request in 9b19880.

malfet pushed a commit to malfet/pytorch that referenced this pull request Nov 20, 2020
Summary:
Inspired by pytorch#47993, this fixes the import error in `collect_env.py` with older version of PyTorch when `torch.version` does not have `hip` property.

Pull Request resolved: pytorch#48076

Reviewed By: seemethere, xuzhao9

Differential Revision: D25024352

Pulled By: samestep

fbshipit-source-id: 7dff9d2ab80b0bd25f9ca035d8660f38419cdeca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla signed Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants