Skip to content

Refactor to reduce duplicate logic in torch._ops#96302

Closed
ezyang wants to merge 4 commits intogh/ezyang/1892/basefrom
gh/ezyang/1892/head
Closed

Refactor to reduce duplicate logic in torch._ops#96302
ezyang wants to merge 4 commits intogh/ezyang/1892/basefrom
gh/ezyang/1892/head

Conversation

@ezyang
Copy link
Contributor

@ezyang ezyang commented Mar 8, 2023

Signed-off-by: Edward Z. Yang <[email protected]>

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Mar 8, 2023

🔗 Helpful Links

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

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

✅ No Failures

As of commit 23a2bd7:
💚 Looks good so far! There are no failures yet. 💚

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

Signed-off-by: Edward Z. Yang <ezyangmeta.com>

[ghstack-poisoned]
Signed-off-by: Edward Z. Yang <ezyangmeta.com>

[ghstack-poisoned]
@ezyang ezyang added the topic: not user facing topic category label Mar 8, 2023
Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

Cool!

type hints and lint warnings are real

# NB: This name is hard-coded in torch/csrc/autograd/python_variable.cpp
# for use with OpOverload; cache lookup is done entirely from C++
# for speed.
# TODO: The cache is NOT currently used by PyOperator, but it should!
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: file issue when you merge this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed later in the stack!


def py_impl(self, dispatch_key, fn):
pass
def py_impl(self, k):
Copy link
Contributor

@zou3519 zou3519 Mar 8, 2023

Choose a reason for hiding this comment

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

nit: k -> key for readability? Though i guess it's not really a key anymore, it's either a key, python mode, or a transform type

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, the old name was really long and so I figured let's just use a short meaningless identifier

Signed-off-by: Edward Z. Yang <ezyangmeta.com>

[ghstack-poisoned]
@albanD albanD removed their request for review March 8, 2023 20:07
cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 12, 2023
cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 12, 2023
ydwu4 added a commit to ydwu4/pytorch that referenced this pull request Mar 13, 2023
@facebook-github-bot facebook-github-bot deleted the gh/ezyang/1892/head branch June 8, 2023 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants