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

Switched from parameter in can_cast to from_. #126030

Closed
wants to merge 1 commit into from
Closed

Switched from parameter in can_cast to from_. #126030

wants to merge 1 commit into from

Conversation

tringwald
Copy link
Collaborator

@tringwald tringwald commented May 12, 2024 •
edited by pytorch-bot bot

Fixes #126012.

from is a reserved keyword in Python, thus we can't make the C++ impl available with from as function parameter. This PR changes the name to from_ and also adjusts the docs.

If we want to preserve backwards compatibility, we can leave the C++ name as-is and only fix the docs. However, torch.can_cast(from_=torch.int, to=torch.int) won't work then.

cc @ezyang @gchanan @albanD

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented May 12, 2024 •
edited

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/126030

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit aca228e with merge base d61a81a (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@ezyang ezyang added the module: bc-breaking Related to a BC-breaking change label May 14, 2024
@ezyang ezyang added triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module and removed topic: bc_breaking labels May 14, 2024
@ezyang ezyang self-requested a review May 14, 2024 17:58
@ezyang
Copy link
Contributor

ezyang commented May 14, 2024

Thank you for the PR, I appreciate it. I'm not sure what the most BC friendly way to fix this is, if you want to land this as is, you will need to update the BC script to whitelist this as a BC breaking change. I think I'm willing to give directly landing this as is a try.

@ezyang ezyang requested a review from albanD May 14, 2024 18:00
albanD
albanD previously approved these changes May 14, 2024
View reviewed changes
Copy link
Collaborator

@albanD albanD left a comment •
edited

Choose a reason for hiding this comment

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

This is BC-breaking for the C++ API but I think it's fine. Not bc-breaking in python because using from= is a syntax error.

The c++ bc script needs updating though

@tringwald
Copy link
Collaborator Author

I updated the BC checking script and will merge when CI is green. I also think the change should be fine, as on the C++ side we can't call by keyword and on the Python side it was a syntax error until now.

@ezyang
Copy link
Contributor

ezyang commented May 15, 2024

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label May 15, 2024
@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: This PR needs a release notes: label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team Raised by workflow job

@tringwald tringwald added the module: python frontend For issues relating to PyTorch's Python frontend label May 15, 2024
@ezyang ezyang added release notes: python_frontend release notes category topic: bug fixes topic category labels May 16, 2024
@ezyang
Copy link
Contributor

ezyang commented May 16, 2024

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@huydhn huydhn mentioned this pull request May 16, 2024
Closed
@huydhn
Copy link
Contributor

huydhn commented May 16, 2024

@pytorchbot revert -m 'Sorry for reverting your change but i need to revert it to avoid a diff train conflict with #125995. Please help rebase and I will reland the change' -c weird

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

pytorchmergebot added a commit that referenced this pull request May 16, 2024
@pytorchmergebot
Revert "Switched from parameter in can_cast to from_. (#126030)"
6065a4d 
This reverts commit 06d6bb4.

Reverted #126030 on behalf of https://github.com/huydhn due to Sorry for reverting your change but i need to revert it to avoid a diff train conflict with #125995.  Please help rebase and I will reland the change ([comment](#126030 (comment)))
@pytorchmergebot
Copy link
Collaborator

@tringwald your PR has been successfully reverted.

@pytorch-bot pytorch-bot bot dismissed albanD’s stale review May 16, 2024 01:42

This PR was reopened (likely due to being reverted), so your approval was removed. Please request another review.

@tringwald tringwald removed the module: python frontend For issues relating to PyTorch's Python frontend label May 16, 2024
@tringwald
Copy link
Collaborator Author

@huydhn The conflict should be resolved now. Can we reland this again?

@ezyang
Copy link
Contributor

ezyang commented May 16, 2024

@pytorchbot merge

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented May 16, 2024

This PR needs to be approved by an authorized maintainer before merge.

@tringwald tringwald requested a review from albanD May 16, 2024 20:53
@albanD
Copy link
Collaborator

albanD commented May 16, 2024

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

ZelboK pushed a commit to ZelboK/pytorch that referenced this pull request May 19, 2024
@tringwald @ZelboK
Switched from parameter in can_cast to from_. (pytorch#126030)
fed9d93 
Fixes pytorch#126012.

`from` is a reserved keyword in Python, thus we can't make the C++ impl available with `from` as function parameter. This PR changes the name to `from_` and also adjusts the docs.

If we want to preserve backwards compatibility, we can leave the C++ name as-is and only fix the docs. However, `torch.can_cast(from_=torch.int, to=torch.int)` won't work then.

Pull Request resolved: pytorch#126030
Approved by: https://github.com/albanD
ZelboK pushed a commit to ZelboK/pytorch that referenced this pull request May 19, 2024
@pytorchmergebot @ZelboK
Revert "Switched from parameter in can_cast to from_. (pytorch#126030)"
930e757 
This reverts commit 06d6bb4.

Reverted pytorch#126030 on behalf of https://github.com/huydhn due to Sorry for reverting your change but i need to revert it to avoid a diff train conflict with pytorch#125995.  Please help rebase and I will reland the change ([comment](pytorch#126030 (comment)))
ZelboK pushed a commit to ZelboK/pytorch that referenced this pull request May 19, 2024
@tringwald @ZelboK
Switched from parameter in can_cast to from_. (pytorch#126030)
f4daf9e 
Fixes pytorch#126012.

`from` is a reserved keyword in Python, thus we can't make the C++ impl available with `from` as function parameter. This PR changes the name to `from_` and also adjusts the docs.

If we want to preserve backwards compatibility, we can leave the C++ name as-is and only fix the docs. However, `torch.can_cast(from_=torch.int, to=torch.int)` won't work then.

Pull Request resolved: pytorch#126030
Approved by: https://github.com/albanD
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albanD albanD albanD approved these changes

@ezyang ezyang Awaiting requested review from ezyang

Assignees
No one assigned
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged module: bc-breaking Related to a BC-breaking change open source release notes: python_frontend release notes category Reverted topic: bug fixes topic category 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.

can_cast() with from argument doesn't work but can_cast() without from argument works
6 participants
@tringwald @ezyang @pytorchmergebot @huydhn @albanD @pytorchbot