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.

Sign up for GitHub

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

Jump to bottom

Change ATEN generator argument type to const std::optional<Generator>& #6595

Closed
wants to merge 2 commits into from
Closed

Change ATEN generator argument type to const std::optional<Generator>& #6595

wants to merge 2 commits into from

Conversation

cyyever
Copy link
Collaborator

@cyyever cyyever commented Feb 23, 2024 •
edited
Loading

To adopt the changes in pytorch/pytorch#120076

@cyyever
Copy link
Collaborator Author

cyyever commented Feb 23, 2024

@alanwaketan Please help review it

@cyyever cyyever requested a review from alanwaketan February 26, 2024 11:35
@cyyever cyyever changed the title Introduce a temporary macro to move to new ATEN api Move to new ATEN api Feb 29, 2024
@cyyever cyyever changed the title Move to new ATEN api Change ATEN generator argument type to const std::optional<Generator>& Feb 29, 2024
@alanwaketan
Copy link
Collaborator

Great, looks like all tests are passed. Please ping me again once the upstream PR is approved. Also, we need to remove the .torch_pin before landing the PR.

@cyyever
Copy link
Collaborator Author

cyyever commented Mar 5, 2024

Great, looks like all tests are passed. Please ping me again once the upstream PR is approved. Also, we need to remove the .torch_pin before landing the PR.

The upstream PR has been approved. Should we first merge the upstream PR or this PR?

@alanwaketan
Copy link
Collaborator

Great, looks like all tests are passed. Please ping me again once the upstream PR is approved. Also, we need to remove the .torch_pin before landing the PR.

The upstream PR has been approved. Should we first merge the upstream PR or this PR?

The upstream PR.

alanwaketan
alanwaketan approved these changes Mar 5, 2024
View reviewed changes
Copy link
Collaborator

@alanwaketan alanwaketan left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for making the companion change!

@cyyever
Copy link
Collaborator Author

cyyever commented Mar 6, 2024 •
edited
Loading

@alanwaketan I have a confusion. If this PR is not merged first, I can't get a valid commit hash in XLA to be referred in the upstream PR for passing its XLA test. Obviously what I can obtain is the commit hash from my repositary.

@alanwaketan alanwaketan mentioned this pull request Mar 6, 2024
Open
@alanwaketan
Copy link
Collaborator

Oh, I forgot that xla hash doesn't work for forked pull request. Do you mind if I took the PR and make a new PR through branching?

@cyyever
Copy link
Collaborator Author

cyyever commented Mar 7, 2024

Oh, I forgot that xla hash doesn't work for forked pull request. Do you mind if I took the PR and make a new PR through branching?

Please help me.

@alanwaketan alanwaketan mentioned this pull request Mar 7, 2024
Merged
@alanwaketan
Copy link
Collaborator

Close this in favor of #6686

@alanwaketan alanwaketan closed this Mar 7, 2024
alanwaketan added a commit that referenced this pull request Mar 7, 2024
@alanwaketan
Change ATEN generator argument type to const std::optional<Generator>& (
8078b8f 
#6686)

Co-authored-by: cyy <[email protected]>

Summary:
To adopt the changes in pytorch/pytorch#120076

To be noted, the original author is @cyyever and this is copied from #6595

Test Plan:
CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alanwaketan alanwaketan alanwaketan approved these changes

Assignees
No one assigned
Labels
REMOVE_TORCH_PIN
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cyyever @alanwaketan