Skip to content

Generalize sizes and strides policy on _make_wrapper_subclass#78646

Closed
suo wants to merge 3 commits intogh/suo/533/basefrom
gh/suo/533/head
Closed

Generalize sizes and strides policy on _make_wrapper_subclass#78646
suo wants to merge 3 commits intogh/suo/533/basefrom
gh/suo/533/head

Conversation

@suo
Copy link
Member

@suo suo commented Jun 1, 2022 •
edited
Loading

Stack from ghstack:

Previously, there was a dispatch_strides boolean arg. Change this to
a string argument that directly maps onto SizesStridesPolicy.

@suo
Generalize sizes and strides policy on _make_wrapper_subclass
76601f3 
Previously, there was a `dispatch_strides` boolean arg. Change this to
a string argument that directly maps onto `SizesStridesPolicy`.

[ghstack-poisoned]
@suo suo requested review from albanD and soulitzer as code owners June 1, 2022 19:09
@facebook-github-bot
Copy link
Contributor

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

🔗 Helpful links

✅ No Failures (0 Pending)

As of commit 09a5e09 (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.

@suo suo requested a review from ezyang June 1, 2022 19:09
@suo
Update on "Generalize sizes and strides policy on _make_wrapper_subcl…
5757556 
…ass"

Previously, there was a `dispatch_strides` boolean arg. Change this to
a string argument that directly maps onto `SizesStridesPolicy`.

[ghstack-poisoned]
albanD
albanD reviewed Jun 1, 2022
View reviewed changes
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

Should we have at least one test for the size case?

torch/csrc/autograd/python_variable.cpp
false,
"Unknown sizes_strides_policy: ",
arg,
"; expected 'strides' or 'sizes'");
Copy link
Collaborator

@albanD albanD Jun 1, 2022

Choose a reason for hiding this comment

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

Is there some doc about what each values does?

Copy link
Member Author

@suo suo Jun 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Right now sizes does not actually dispatch back into Python. Forthcoming PRs stacked on top will implement this and add tests.

Is there some doc about what each values does?

Ummmm, there's a big comment in tensorimpl

Copy link
Collaborator

@albanD albanD Jun 1, 2022

Choose a reason for hiding this comment

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

But that's quite far from that python API :p
Also there is a small mismatch between what it changes for the TensorImpl and what ends up being called on __torch_dispatch__

Copy link
Member Author

@suo suo Jun 1, 2022

Choose a reason for hiding this comment

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

I can add documentation for it, where is are the docs for _make_wrapper_subclass/_make_subclass today?

As for the mismatch—I don't think that's fixable within the scope of this PR (and it already exists). But hopefully we'll finish coverage of the various tensorimpl methods soon and so at least we can document it?

Copy link
Collaborator

@albanD albanD Jun 1, 2022

Choose a reason for hiding this comment

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

I don't think there is any code that needs changing. Just mentioning that because they work at very different levels the doc in TensorImpl might not be enough?

I can add documentation for it, where is are the docs for _make_wrapper_subclass/_make_subclass today?

For this, I think the TORCH_CHECK is a good place.

Copy link
Member Author

@suo suo Jun 2, 2022

Choose a reason for hiding this comment

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

hmmmm, I feel like it's weird to put the rules for what this argument does in the error message; we should probably just have like a real docstring?

Okay I'm going to merge this now, but I will take as a follow up:

  • Finding the right place to describe the rules
  • Making the code actually reflect the rules (e.g. make more things overridable from torchdispatch)

@suo
Update on "Generalize sizes and strides policy on _make_wrapper_subcl…
09a5e09 
…ass"

Previously, there was a `dispatch_strides` boolean arg. Change this to
a string argument that directly maps onto `SizesStridesPolicy`.

[ghstack-poisoned]
suo added a commit that referenced this pull request Jun 1, 2022
@suo
Generalize sizes and strides policy on _make_wrapper_subclass
3d813c6 
Previously, there was a `dispatch_strides` boolean arg. Change this to
a string argument that directly maps onto `SizesStridesPolicy`.

ghstack-source-id: b115363
Pull Request resolved: #78646
@suo
Copy link
Member Author

suo commented Jun 2, 2022

@pytorchbot help

@pytorch-bot
Copy link

pytorch-bot bot commented Jun 2, 2022

PyTorchBot Help

usage: @pytorchbot {merge,revert,rebase,help} ...

command:
  {merge,revert,rebase,help}
    merge               Merge a PR
    revert              Revert a merged PR
    rebase              Rebase a PR
    help                Show help

Merge

usage: @pytorchbot merge [-g] [-a] [-f]

optional arguments:
  -g, --green      Merge when required tests pass
  -a, --all-green  Merge when all tests pass
  -f, --force      Merge without checking anything

Revert

usage: @pytorchbot revert -m MESSAGE
                          [-c {nosignal,ignoredsignal,landrace,weird,ghfirst}]

optional arguments:
  -m MESSAGE, --message MESSAGE
                        The reason you are reverting, will be put in the
                        commit message.
  -c {nosignal,ignoredsignal,landrace,weird,ghfirst}, --classification {nosignal,ignoredsignal,landrace,weird,ghfirst}
                        A machine-friendly classification of the revert
                        reason.

Rebase

usage: @pytorchbot rebase [-s]

optional arguments:
  -s, --stable  Rebase to viable/strict

For more info, consult the wiki."

@suo
Copy link
Member Author

suo commented Jun 2, 2022

@pytorchbot merge -g

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2022

Hey @suo.
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.

@suo suo mentioned this pull request Jun 2, 2022
Closed
facebook-github-bot pushed a commit that referenced this pull request Jun 2, 2022
@suo @facebook-github-bot
Generalize sizes and strides policy on _make_wrapper_subclass (#78646)
1c5e58d 
Summary:
Previously, there was a `dispatch_strides` boolean arg. Change this to
a string argument that directly maps onto `SizesStridesPolicy`.

Pull Request resolved: #78646

Approved by: https://github.com/ezyang

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

Reviewed By: b0noI

Differential Revision: D36854294

Pulled By: suo

fbshipit-source-id: c535498d1c5ed88c6cdb40926fb49ced065907a5
@facebook-github-bot facebook-github-bot deleted the gh/suo/533/head branch June 5, 2022 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@albanD albanD albanD left review comments

@ezyang ezyang ezyang approved these changes

@soulitzer soulitzer Awaiting requested review from soulitzer

Assignees

No one assigned

Labels

cla signed Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@suo @facebook-github-bot @ezyang @albanD @pytorchmergebot