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

Don't call item() into torch.scalar_tensor uselessly #125373

Closed
wants to merge 6 commits into from
Closed

Don't call item() into torch.scalar_tensor uselessly #125373

wants to merge 6 commits into from
+15 −3

Conversation

ezyang
Copy link
Contributor

@ezyang ezyang commented May 2, 2024 •
edited by pytorch-bot bot

@ezyang
Update
3f06893 
[ghstack-poisoned]
@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented May 2, 2024 •
edited

🔗 Helpful Links

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

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

✅ No Failures

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

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

ezyang added a commit that referenced this pull request May 2, 2024
@ezyang
Don't call item() into torch.scalar_tensor uselessly
f057766 
Fixes #125368

Signed-off-by: Edward Z. Yang <ezyang@meta.com>

ghstack-source-id: db51d1ed981a7e9edcd5342993270be4d02728f9
Pull Request resolved: #125373
@ezyang
Update
bd17001 
[ghstack-poisoned]
ezyang added a commit that referenced this pull request May 2, 2024
@ezyang
Don't call item() into torch.scalar_tensor uselessly
e0e7b9f 
Fixes #125368

Signed-off-by: Edward Z. Yang <ezyang@meta.com>

ghstack-source-id: e273a9bb1704b4481894bc334695bd005cbb7ede
Pull Request resolved: #125373
Skylion007
Skylion007 approved these changes May 2, 2024
View reviewed changes
Copy link
Collaborator

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

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

Looks good assuming you can fix the implicit casting of float Tensor to Short

albanD
albanD reviewed May 2, 2024
View reviewed changes
torch/_refs/__init__.py Outdated
obj = obj.item()
# fall through into next case
if isinstance(obj, Number):
return obj.to(dtype=scalarType, device="cpu", copy=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the scalar_tensor() constructor setting some special flag that this would not?
Also do we need a .squeeze() to collapse all the dimensions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not, scalar_tensor is basically the same as tensor (but it can only make 0d tensors). And yes, need the squeeze.

@ezyang
Update
f5527e1 
[ghstack-poisoned]
ezyang added a commit that referenced this pull request May 4, 2024
@ezyang
Don't call item() into torch.scalar_tensor uselessly
8b67427 
Fixes #125368

Signed-off-by: Edward Z. Yang <ezyang@meta.com>

ghstack-source-id: 674413cd6a982bf5da4b51e11acba8a26736c7c4
Pull Request resolved: #125373
@ezyang ezyang added release notes: composability release notes category topic: bug fixes topic category labels May 4, 2024
@ezyang
Copy link
Contributor Author

ezyang commented May 4, 2024

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label May 4, 2024
@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
Copy link
Contributor

huydhn commented May 4, 2024

@pytorchbot drci

@huydhn
Copy link
Contributor

huydhn commented May 4, 2024

@pytorchbot revert -m 'Sorry for reverting your change, but there are real failures on the PR that sneak in during the log classifier outage' -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 4, 2024
@pytorchmergebot
Revert "Don't call item() into torch.scalar_tensor uselessly (#125373)"
a32ad82 
This reverts commit 2b4fe18.

Reverted #125373 on behalf of https://github.com/huydhn due to Sorry for reverting your change, but there are real failures on the PR that sneak in during the log classifier outage ([comment](#125373 (comment)))
@pytorchmergebot
Copy link
Collaborator

@ezyang your PR has been successfully reverted.

@github-actions github-actions bot requested a review from albanD May 4, 2024 22:22
@ezyang
Update
ad1e8ec 
[ghstack-poisoned]
ezyang added a commit that referenced this pull request May 5, 2024
@ezyang
Don't call item() into torch.scalar_tensor uselessly
ee31c3a 
Fixes #125368

Signed-off-by: Edward Z. Yang <ezyang@meta.com>

ghstack-source-id: 1213017b4dedda23c96e771df52b040e8a0a7ef9
Pull Request resolved: #125373
@ezyang ezyang mentioned this pull request May 5, 2024
Closed
@ezyang
Copy link
Contributor Author

ezyang commented May 5, 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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 4 mandatory check(s) failed. The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

@ezyang
Update
d25225e 
[ghstack-poisoned]
ezyang added a commit that referenced this pull request May 5, 2024
@ezyang
Don't call item() into torch.scalar_tensor uselessly
994ef55 
Fixes #125368

Signed-off-by: Edward Z. Yang <ezyang@meta.com>

ghstack-source-id: 9df853271d36d15f69d0c83214d58c3c84d24e2e
Pull Request resolved: #125373
@ezyang
Update
35dea73 
[ghstack-poisoned]
ezyang added a commit that referenced this pull request May 5, 2024
@ezyang
Don't call item() into torch.scalar_tensor uselessly
8c2d8a8 
Fixes #125368

Signed-off-by: Edward Z. Yang <ezyang@meta.com>

ghstack-source-id: 00084245e72af57b7e034a8e2bff3e0d881e630e
Pull Request resolved: #125373
@ezyang
Copy link
Contributor Author

ezyang commented May 5, 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

@github-actions github-actions bot deleted the gh/ezyang/2720/head branch June 5, 2024 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Skylion007 Skylion007 Skylion007 approved these changes

@antoniojkim antoniojkim Awaiting requested review from antoniojkim

@bdhirsh bdhirsh Awaiting requested review from bdhirsh

@miladm miladm Awaiting requested review from miladm

@SherlockNoMad SherlockNoMad Awaiting requested review from SherlockNoMad

@avikchaudhuri avikchaudhuri Awaiting requested review from avikchaudhuri

@IvanKobzarev IvanKobzarev Awaiting requested review from IvanKobzarev

@albanD albanD Awaiting requested review from albanD

Assignees
No one assigned
Labels
ci-td-distributed ciflow/trunk Trigger trunk jobs on your pull request Merged module: dynamo release notes: composability release notes category Reverted topic: bug fixes topic category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ezyang @pytorchmergebot @huydhn @Skylion007 @albanD