Skip to content

add layout to slow path #80429

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
Closed
wants to merge 13 commits into from
Closed

add layout to slow path #80429

wants to merge 13 commits into from

Conversation

george-qi
Copy link
Contributor

@george-qi george-qi commented Jun 28, 2022 •
edited
Loading

@george-qi
add layout to slow path
70b7b0e 
[ghstack-poisoned]
george-qi added a commit that referenced this pull request Jun 28, 2022
@george-qi
add layout to slow path
ea9c283 
ghstack-source-id: dc1787e
Pull Request resolved: #80429
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jun 28, 2022 •
edited
Loading

🔗 Helpful links

✅ No Failures (0 Pending)

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

Expand to see more

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

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jun 28, 2022
@albanD albanD removed their request for review June 28, 2022 13:41
george-qi added a commit that referenced this pull request Jun 29, 2022
@george-qi
add layout to slow path
0cde02d 
ghstack-source-id: 862aeb3
Pull Request resolved: #80429
george-qi added a commit that referenced this pull request Jun 30, 2022
@george-qi
add layout to slow path
d08b94d 
ghstack-source-id: 4e8b094
Pull Request resolved: #80429
@george-qi george-qi added the ciflow/trunk Trigger trunk jobs on your pull request label Jun 30, 2022
@george-qi george-qi requested a review from ezyang June 30, 2022 01:23
@george-qi george-qi requested a review from zou3519 June 30, 2022 01:23
@ezyang
Copy link
Contributor

ezyang commented Jun 30, 2022

While this PR is logical, it is not clear to me why it is necessary. Can you elaborate more on the use case?

@george-qi
Copy link
Contributor Author

While this PR is logical, it is not clear to me why it is necessary. Can you elaborate more on the use case?

Yep! We’re trying to implement sparse CSR support in MaskedTensor (see pytorch/maskedtensor#65), and while performing operations like .to_dense(), it ends up calling layout(), which now reroutes to the not implemented virtual function layout_impl() and errors out

ezyang
ezyang reviewed Jul 4, 2022
View reviewed changes
torch/csrc/utils/python_arg_parser.h Outdated
}
const auto layout = THPUtils_unpackLong(obj);
TORCH_CHECK(layout >= 0, "Layout must not be negative");
return static_cast<at::Layout>(layout);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the thing where __torch_dispatch__ gives you an integer instead of the layout object, right? This should get fixed by https://github.com/pytorch/pytorch/pull/77543/files Do you still need this logic?

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, this doesn't do any checking that the integer corresponds to a real layout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think you're right - I don't think it's needed, was put there more just in case. I'll remove it!

george-qi added a commit that referenced this pull request Jul 5, 2022
@george-qi
add layout to slow path
564bd47 
ghstack-source-id: bb95d13
Pull Request resolved: #80429
@george-qi
Copy link
Contributor Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a merge job. Check the current status here

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2022

Hey @george-qi.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

facebook-github-bot pushed a commit that referenced this pull request Jul 8, 2022
@george-qi @facebook-github-bot
add layout to slow path (#80429) (#80429)
6fd6b17 
Summary:
Pull Request resolved: #80429
Approved by: https://github.com/ezyang

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/393f7f6ad7b10adc44500eaead13044efd8c698b

Reviewed By: mehtanirav

Differential Revision: D37687406

Pulled By: george-qi

fbshipit-source-id: 62e53ac714642d9582719377c0ef8082e383dc3a
@facebook-github-bot facebook-github-bot deleted the gh/george-qi/32/head branch July 10, 2022 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ezyang ezyang ezyang approved these changes

@soulitzer soulitzer Awaiting requested review from soulitzer

@zou3519 zou3519 Awaiting requested review from zou3519

Assignees

No one assigned

Labels

ciflow/trunk Trigger trunk jobs on your pull request cla signed Merged oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@george-qi @facebook-github-bot @ezyang @pytorchmergebot