Skip to content

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

Closed
cyyever wants to merge 2 commits intopytorch:masterfrom
cyyever:const_generator
Closed

Change ATEN generator argument type to const std::optional<Generator>&#6595
cyyever wants to merge 2 commits intopytorch:masterfrom
cyyever:const_generator

Conversation

@cyyever
Copy link
Copy Markdown
Collaborator

@cyyever cyyever commented Feb 23, 2024 •
edited
Loading

To adopt the changes in pytorch/pytorch#120076

@cyyever
Copy link
Copy Markdown
Collaborator Author

cyyever commented Feb 23, 2024

@alanwaketan Please help review it

alanwaketan
alanwaketan reviewed Feb 23, 2024
View reviewed changes
Comment thread torch_xla/csrc/aten_xla_type.cpp Outdated
@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
Copy Markdown
Collaborator

alanwaketan 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.

@cyyever
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

alanwaketan 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?

The upstream PR.

alanwaketan
alanwaketan approved these changes Mar 5, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

alanwaketan commented Mar 6, 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?

@cyyever
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

alanwaketan commented Mar 7, 2024

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 <cyyever@outlook.com>

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