Skip to content

ROCm: enable cpp_extensions.load/load_inline#35897

Closed
t-vi wants to merge 11 commits intopytorch:masterfrom
t-vi:hip_cpp_extension_load
Closed

ROCm: enable cpp_extensions.load/load_inline#35897
t-vi wants to merge 11 commits intopytorch:masterfrom
t-vi:hip_cpp_extension_load

Conversation

@t-vi
Copy link
Collaborator

@t-vi t-vi commented Apr 2, 2020

This enables cpp_extensions.load/load_inline. This works by hipify-ing cuda sources.
Also enable tests.
CuDNN/MIOpen extensions aren't yet supported, I propose to not do this in this PR.

@dr-ci
Copy link

dr-ci bot commented Apr 2, 2020 •
edited
Loading

💊 CircleCI build failures summary and remediations

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

💚 💚 Looks good so far! There are no CircleCI failures yet. 💚 💚

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.

See how this bot performed.

This comment has been revised 32 times.

@t-vi
Copy link
Collaborator Author

t-vi commented Apr 3, 2020

From what I can see, the errors seem to be mostly network connectivity rather than what happens in the relevant tests.

ezyang
ezyang reviewed Apr 3, 2020
View reviewed changes
ezyang
ezyang reviewed Apr 3, 2020
View reviewed changes
ezyang
ezyang reviewed Apr 3, 2020
View reviewed changes
ezyang
ezyang reviewed Apr 3, 2020
View reviewed changes
ezyang
ezyang reviewed Apr 3, 2020
View reviewed changes
ezyang
ezyang requested changes Apr 3, 2020
View reviewed changes
Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

some minor bugs

@t-vi
Copy link
Collaborator Author

t-vi commented Apr 3, 2020

Thank you for the prompt review, @ezyang !
I changed to keep the intermediates, that also gets rid of some of the other uglyness.
Fixed the other bits.

@zou3519 zou3519 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Apr 6, 2020
@t-vi
Copy link
Collaborator Author

t-vi commented Apr 7, 2020

So if you're OK with keeping the generated hip (after a bit more thought, I think it is preferable), this should have the things fixed.

ezyang
ezyang reviewed Apr 7, 2020
View reviewed changes
ezyang
ezyang reviewed Apr 7, 2020
View reviewed changes
ezyang
ezyang reviewed Apr 9, 2020
View reviewed changes
ezyang
ezyang reviewed Apr 9, 2020
View reviewed changes
@t-vi
Copy link
Collaborator Author

t-vi commented Apr 10, 2020

It the commentary OK now?

@ezyang
Copy link
Contributor

ezyang commented Apr 10, 2020

Lint.


  {
    path: 'torch/utils/hipify/hipify_python.py',
    start_line: 823,
    end_line: 823,
    start_column: 17,
    end_column: 17,
    annotation_level: 'failure',
    message: '[B006] Do not use mutable data structures for argument defaults.  They are created during function definition time. All calls to the function reuse this one instance of that data structure, persisting changes between them.'
  }

@t-vi
it should be don't mutate default arguments rather than don't have mu…
67c784d 
…tuable default arguments but who cares about nuances
@t-vi
Copy link
Collaborator Author

t-vi commented Apr 10, 2020

Oh. Sorry about that. 🤦‍♂️

facebook-github-bot
facebook-github-bot reviewed Apr 11, 2020
View reviewed changes
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.

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

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Apr 13, 2020

@ezyang merged this pull request in d070c0b.

@Gamrix Gamrix mentioned this pull request Aug 10, 2022
Closed
@ZJLi2013 ZJLi2013 mentioned this pull request Oct 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ezyang ezyang ezyang requested changes

@fmassa fmassa Awaiting requested review from fmassa

@goldsborough goldsborough Awaiting requested review from goldsborough

@soumith soumith Awaiting requested review from soumith

+2 more reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@iotamudelta iotamudelta iotamudelta approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@t-vi @ezyang @facebook-github-bot @iotamudelta @zou3519 @pytorchbot @mruberry