Skip to content

Use is, not ==, to check exact type matches in _python_dispatch #161304

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 6 commits into from
Closed

Use is, not ==, to check exact type matches in _python_dispatch #161304

wants to merge 6 commits into from

Conversation

@swolchok
Copy link
Contributor

@swolchok swolchok commented Aug 22, 2025 •
edited
Loading

Stack from ghstack (oldest at bottom):

is checks object identity and is more efficient. Google seems to confirm it is the correct way to do an exact type check.

@swolchok
Use is, not ==, to check exact type matches in _python_dispatch
a7f61aa 
`is` checks object identity and is more efficient. Google seems to confirm it is the correct way to do an exact type check.

[ghstack-poisoned]
This was referenced Aug 22, 2025
Closed
Closed
@pytorch-bot
Copy link

pytorch-bot bot commented Aug 22, 2025 •
edited
Loading

🔗 Helpful Links

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

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

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

As of commit 0c4467c with merge base 05eeb29 (image):
💚 Looks good so far! There are no failures yet. 💚

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

This was referenced Aug 22, 2025
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
swolchok added a commit that referenced this pull request Aug 22, 2025
@swolchok
Use is, not ==, to check exact type matches in _python_dispatch
0b875ac 
`is` checks object identity and is more efficient. Google seems to confirm it is the correct way to do an exact type check.

ghstack-source-id: c915db0
Pull Request resolved: #161304
@swolchok swolchok mentioned this pull request Aug 22, 2025
Closed
@swolchok swolchok requested review from laithsakka and wanchaol and removed request for wanchaol August 22, 2025 21:59
@swolchok swolchok added the topic: not user facing topic category label Aug 22, 2025
@swolchok
Update on "Use is, not ==, to check exact type matches in _python_d…
85af02a 
…ispatch"

`is` checks object identity and is more efficient. Google seems to confirm it is the correct way to do an exact type check.

[ghstack-poisoned]
@swolchok swolchok mentioned this pull request Aug 22, 2025
Closed
Skylion007
Skylion007 approved these changes Aug 22, 2025
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.

We have a flake8/ruff rule for this btw, but it's not enabled.

@swolchok swolchok mentioned this pull request Aug 22, 2025
Closed
@swolchok swolchok requested review from XilunWu, ezyang and zpcore August 22, 2025 22:31
This was referenced Aug 23, 2025
Closed
Closed
Skylion007
Skylion007 reviewed Aug 23, 2025
View reviewed changes
torch/utils/_python_dispatch.py
return (
issubclass(t, torch.Tensor)
and t != torch.Tensor
and t is not torch.Tensor
Copy link
Collaborator

@Skylion007 Skylion007 Aug 23, 2025

Choose a reason for hiding this comment

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

Ruff rule is E721 and can be silenced by either using isinstance or type is. We should enable it after we migrate all the instances. We are pretty consistent about not wanting to handle subclasses in a lot of inductor so mostly should be "is" overloads.

@swolchok
Update on "Use is, not ==, to check exact type matches in _python_d…
0c4467c 
…ispatch"

`is` checks object identity and is more efficient. Google seems to confirm it is the correct way to do an exact type check.

[ghstack-poisoned]
@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Aug 30, 2025

Starting merge as part of PR stack under #161432

1 similar comment
@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Aug 30, 2025

Starting merge as part of PR stack under #161432

pytorchmergebot pushed a commit that referenced this pull request Aug 30, 2025
@swolchok @pytorchmergebot
Optimize _python_dispatch.return_and_correct_aliasing.get_write_alias (…
b96bcb9 
…#161308)

- Empty containers are Falsey
- Hoist cheap checks first
- Microbenchmarked single-element set access method

Benchmark code:
```
import timeit

to_test = [
    ('list(x)', 'x = set([3])'),
    ('x[0]', 'x = [3]'),
    ('list(x)[0]', 'x = set([3])'),
    ('next(iter(x))', 'x = set([3])'),
]

for (stmt, setup) in to_test:
    res = timeit.timeit(stmt=stmt, setup=setup)
    print(f"Time for `{stmt}`: {res}")
```

Result with Python 3.13 on Mac (with excess digits manually trimmed; directionally matches result on Linux)
```
Time for `list(x)`: 0.03418
Time for `x[0]`: 0.00852
Time for `list(x)[0]`: 0.03561
Time for `next(iter(x))`: 0.02278
```

FWIW, I was surprised by this result, so I guess I'm glad I wrote the benchmark!
Pull Request resolved: #161308
Approved by: https://github.com/Skylion007, https://github.com/bdhirsh
ghstack dependencies: #161301, #161292, #161304
pytorchmergebot pushed a commit that referenced this pull request Aug 30, 2025
@swolchok @pytorchmergebot
Fix pybind enum efficiency issue in return_and_correct_aliasing (#161315
0c459f2 
)

Scanning a list of pybind enums with `in` is slow. See NOTE in code for full explanation.

This is a significant optimization; will be updating the torchdispatch/return_and_correct_aliasing portion of this stack with benchmark and results soonish.
Pull Request resolved: #161315
Approved by: https://github.com/Skylion007, https://github.com/bdhirsh
ghstack dependencies: #161301, #161292, #161304, #161308
pytorchmergebot pushed a commit that referenced this pull request Aug 30, 2025
@swolchok @pytorchmergebot
Improve assert perf in _python_dispatch._correct_storage_aliasing (#1…
302d860 
…61317)

This assertion was expensive because of is_traceable_wrapper_subclass. Finding a cheap check to run first that's likely to let us skip the rest seems to improve things significantly.
Pull Request resolved: #161317
Approved by: https://github.com/ezyang, https://github.com/XilunWu, https://github.com/bdhirsh
ghstack dependencies: #161301, #161292, #161304, #161308, #161315
pytorchmergebot pushed a commit that referenced this pull request Aug 30, 2025
@swolchok @pytorchmergebot
Avoid double hash lookup in torch._library.simple_registry (#161328)
eb9526a 
Not a huge cost, but free win is free.
Pull Request resolved: #161328
Approved by: https://github.com/Skylion007
ghstack dependencies: #161301, #161292, #161304, #161308, #161315, #161317
pytorchmergebot pushed a commit that referenced this pull request Aug 30, 2025
@swolchok @pytorchmergebot
Fix accidental copy in pushPyOutToStack (#161329)
0ee8a4e 
`auto` forces a copy. Confirmed this did something noticable with perf.
Pull Request resolved: #161329
Approved by: https://github.com/zpcore, https://github.com/fduwjj, https://github.com/Skylion007, https://github.com/bdhirsh
ghstack dependencies: #161301, #161292, #161304, #161308, #161315, #161317, #161328
pytorchmergebot pushed a commit that referenced this pull request Aug 30, 2025
@swolchok @pytorchmergebot
Stop trying to intern arguments in PyObject_FastGetAttrString (#161432)
4d3ab26 
If we want them interned, we should intern at callsites. (The numpy reference has bit rotted; see numpy/numpy@b222eb6#diff-6bdb6105198083838f51c57b55b3a49472ed23043bb40018f1ea41138e687163)

Profiling a simple torchdispatch benchmark with perf before/after seems to show that time spent copying std::strings and interning Python strings is gone, though there is some noise and the improvement is very small.
Pull Request resolved: #161432
Approved by: https://github.com/ezyang
ghstack dependencies: #161301, #161292, #161304, #161308, #161315, #161317, #161328, #161329
pytorchmergebot pushed a commit that referenced this pull request Aug 31, 2025
@swolchok @pytorchmergebot
PythonArgs::toBool: order cheap mutually exclusive checks first (#161455
68738be 
)

symbools are not identical with Py_True or PyFalse, so we can do those cheap checks first and at least get plain old bools to go fast.
Pull Request resolved: #161455
Approved by: https://github.com/Skylion007
ghstack dependencies: #161301, #161292, #161304, #161308, #161315, #161317, #161328, #161329, #161432
markc-614 pushed a commit to markc-614/pytorch that referenced this pull request Sep 17, 2025
@swolchok @markc-614
Use is, not ==, to check exact type matches in _python_dispatch (py…
b57d015 
…torch#161304)

`is` checks object identity and is more efficient. Google seems to confirm it is the correct way to do an exact type check.
Pull Request resolved: pytorch#161304
Approved by: https://github.com/Skylion007, https://github.com/malfet, https://github.com/bdhirsh
ghstack dependencies: pytorch#161301, pytorch#161292
markc-614 pushed a commit to markc-614/pytorch that referenced this pull request Sep 17, 2025
@swolchok @markc-614
Optimize _python_dispatch.return_and_correct_aliasing.get_write_alias (…
bfb903c 
…pytorch#161308)

- Empty containers are Falsey
- Hoist cheap checks first
- Microbenchmarked single-element set access method

Benchmark code:
```
import timeit

to_test = [
    ('list(x)', 'x = set([3])'),
    ('x[0]', 'x = [3]'),
    ('list(x)[0]', 'x = set([3])'),
    ('next(iter(x))', 'x = set([3])'),
]

for (stmt, setup) in to_test:
    res = timeit.timeit(stmt=stmt, setup=setup)
    print(f"Time for `{stmt}`: {res}")
```

Result with Python 3.13 on Mac (with excess digits manually trimmed; directionally matches result on Linux)
```
Time for `list(x)`: 0.03418
Time for `x[0]`: 0.00852
Time for `list(x)[0]`: 0.03561
Time for `next(iter(x))`: 0.02278
```

FWIW, I was surprised by this result, so I guess I'm glad I wrote the benchmark!
Pull Request resolved: pytorch#161308
Approved by: https://github.com/Skylion007, https://github.com/bdhirsh
ghstack dependencies: pytorch#161301, pytorch#161292, pytorch#161304
markc-614 pushed a commit to markc-614/pytorch that referenced this pull request Sep 17, 2025
@swolchok @markc-614
Fix pybind enum efficiency issue in return_and_correct_aliasing (pyto…
650efbd 
…rch#161315)

Scanning a list of pybind enums with `in` is slow. See NOTE in code for full explanation.

This is a significant optimization; will be updating the torchdispatch/return_and_correct_aliasing portion of this stack with benchmark and results soonish.
Pull Request resolved: pytorch#161315
Approved by: https://github.com/Skylion007, https://github.com/bdhirsh
ghstack dependencies: pytorch#161301, pytorch#161292, pytorch#161304, pytorch#161308
markc-614 pushed a commit to markc-614/pytorch that referenced this pull request Sep 17, 2025
@swolchok @markc-614
Improve assert perf in _python_dispatch._correct_storage_aliasing (py…
4b883dd 
…torch#161317)

This assertion was expensive because of is_traceable_wrapper_subclass. Finding a cheap check to run first that's likely to let us skip the rest seems to improve things significantly.
Pull Request resolved: pytorch#161317
Approved by: https://github.com/ezyang, https://github.com/XilunWu, https://github.com/bdhirsh
ghstack dependencies: pytorch#161301, pytorch#161292, pytorch#161304, pytorch#161308, pytorch#161315
markc-614 pushed a commit to markc-614/pytorch that referenced this pull request Sep 17, 2025
@swolchok @markc-614
Avoid double hash lookup in torch._library.simple_registry (pytorch#1…
e0d17bf 
…61328)

Not a huge cost, but free win is free.
Pull Request resolved: pytorch#161328
Approved by: https://github.com/Skylion007
ghstack dependencies: pytorch#161301, pytorch#161292, pytorch#161304, pytorch#161308, pytorch#161315, pytorch#161317
markc-614 pushed a commit to markc-614/pytorch that referenced this pull request Sep 17, 2025
@swolchok @markc-614
Fix accidental copy in pushPyOutToStack (pytorch#161329)
d8e149f 
`auto` forces a copy. Confirmed this did something noticable with perf.
Pull Request resolved: pytorch#161329
Approved by: https://github.com/zpcore, https://github.com/fduwjj, https://github.com/Skylion007, https://github.com/bdhirsh
ghstack dependencies: pytorch#161301, pytorch#161292, pytorch#161304, pytorch#161308, pytorch#161315, pytorch#161317, pytorch#161328
markc-614 pushed a commit to markc-614/pytorch that referenced this pull request Sep 17, 2025
@swolchok @markc-614
Stop trying to intern arguments in PyObject_FastGetAttrString (pytorc…
5bcac1f 
…h#161432)

If we want them interned, we should intern at callsites. (The numpy reference has bit rotted; see numpy/numpy@b222eb6#diff-6bdb6105198083838f51c57b55b3a49472ed23043bb40018f1ea41138e687163)

Profiling a simple torchdispatch benchmark with perf before/after seems to show that time spent copying std::strings and interning Python strings is gone, though there is some noise and the improvement is very small.
Pull Request resolved: pytorch#161432
Approved by: https://github.com/ezyang
ghstack dependencies: pytorch#161301, pytorch#161292, pytorch#161304, pytorch#161308, pytorch#161315, pytorch#161317, pytorch#161328, pytorch#161329
markc-614 pushed a commit to markc-614/pytorch that referenced this pull request Sep 17, 2025
@swolchok @markc-614
PythonArgs::toBool: order cheap mutually exclusive checks first (pyto…
c40c1e7 
…rch#161455)

symbools are not identical with Py_True or PyFalse, so we can do those cheap checks first and at least get plain old bools to go fast.
Pull Request resolved: pytorch#161455
Approved by: https://github.com/Skylion007
ghstack dependencies: pytorch#161301, pytorch#161292, pytorch#161304, pytorch#161308, pytorch#161315, pytorch#161317, pytorch#161328, pytorch#161329, pytorch#161432
mansiag05 pushed a commit to mansiag05/pytorch that referenced this pull request Sep 22, 2025
@swolchok @mansiag05
Use is, not ==, to check exact type matches in _python_dispatch (py…
f4e2f47 
…torch#161304)

`is` checks object identity and is more efficient. Google seems to confirm it is the correct way to do an exact type check.
Pull Request resolved: pytorch#161304
Approved by: https://github.com/Skylion007, https://github.com/malfet, https://github.com/bdhirsh
ghstack dependencies: pytorch#161301, pytorch#161292
mansiag05 pushed a commit to mansiag05/pytorch that referenced this pull request Sep 22, 2025
@swolchok @mansiag05
Optimize _python_dispatch.return_and_correct_aliasing.get_write_alias (…
af8b08a 
…pytorch#161308)

- Empty containers are Falsey
- Hoist cheap checks first
- Microbenchmarked single-element set access method

Benchmark code:
```
import timeit

to_test = [
    ('list(x)', 'x = set([3])'),
    ('x[0]', 'x = [3]'),
    ('list(x)[0]', 'x = set([3])'),
    ('next(iter(x))', 'x = set([3])'),
]

for (stmt, setup) in to_test:
    res = timeit.timeit(stmt=stmt, setup=setup)
    print(f"Time for `{stmt}`: {res}")
```

Result with Python 3.13 on Mac (with excess digits manually trimmed; directionally matches result on Linux)
```
Time for `list(x)`: 0.03418
Time for `x[0]`: 0.00852
Time for `list(x)[0]`: 0.03561
Time for `next(iter(x))`: 0.02278
```

FWIW, I was surprised by this result, so I guess I'm glad I wrote the benchmark!
Pull Request resolved: pytorch#161308
Approved by: https://github.com/Skylion007, https://github.com/bdhirsh
ghstack dependencies: pytorch#161301, pytorch#161292, pytorch#161304
mansiag05 pushed a commit to mansiag05/pytorch that referenced this pull request Sep 22, 2025
@swolchok @mansiag05
Fix pybind enum efficiency issue in return_and_correct_aliasing (pyto…
44a94fa 
…rch#161315)

Scanning a list of pybind enums with `in` is slow. See NOTE in code for full explanation.

This is a significant optimization; will be updating the torchdispatch/return_and_correct_aliasing portion of this stack with benchmark and results soonish.
Pull Request resolved: pytorch#161315
Approved by: https://github.com/Skylion007, https://github.com/bdhirsh
ghstack dependencies: pytorch#161301, pytorch#161292, pytorch#161304, pytorch#161308
mansiag05 pushed a commit to mansiag05/pytorch that referenced this pull request Sep 22, 2025
@swolchok @mansiag05
Improve assert perf in _python_dispatch._correct_storage_aliasing (py…
24dede1 
…torch#161317)

This assertion was expensive because of is_traceable_wrapper_subclass. Finding a cheap check to run first that's likely to let us skip the rest seems to improve things significantly.
Pull Request resolved: pytorch#161317
Approved by: https://github.com/ezyang, https://github.com/XilunWu, https://github.com/bdhirsh
ghstack dependencies: pytorch#161301, pytorch#161292, pytorch#161304, pytorch#161308, pytorch#161315
mansiag05 pushed a commit to mansiag05/pytorch that referenced this pull request Sep 22, 2025
@swolchok @mansiag05
Avoid double hash lookup in torch._library.simple_registry (pytorch#1…
fda5520 
…61328)

Not a huge cost, but free win is free.
Pull Request resolved: pytorch#161328
Approved by: https://github.com/Skylion007
ghstack dependencies: pytorch#161301, pytorch#161292, pytorch#161304, pytorch#161308, pytorch#161315, pytorch#161317
mansiag05 pushed a commit to mansiag05/pytorch that referenced this pull request Sep 22, 2025
@swolchok @mansiag05
Fix accidental copy in pushPyOutToStack (pytorch#161329)
9a7fee1 
`auto` forces a copy. Confirmed this did something noticable with perf.
Pull Request resolved: pytorch#161329
Approved by: https://github.com/zpcore, https://github.com/fduwjj, https://github.com/Skylion007, https://github.com/bdhirsh
ghstack dependencies: pytorch#161301, pytorch#161292, pytorch#161304, pytorch#161308, pytorch#161315, pytorch#161317, pytorch#161328
mansiag05 pushed a commit to mansiag05/pytorch that referenced this pull request Sep 22, 2025
@swolchok @mansiag05
Stop trying to intern arguments in PyObject_FastGetAttrString (pytorc…
e6f5b36 
…h#161432)

If we want them interned, we should intern at callsites. (The numpy reference has bit rotted; see numpy/numpy@b222eb6#diff-6bdb6105198083838f51c57b55b3a49472ed23043bb40018f1ea41138e687163)

Profiling a simple torchdispatch benchmark with perf before/after seems to show that time spent copying std::strings and interning Python strings is gone, though there is some noise and the improvement is very small.
Pull Request resolved: pytorch#161432
Approved by: https://github.com/ezyang
ghstack dependencies: pytorch#161301, pytorch#161292, pytorch#161304, pytorch#161308, pytorch#161315, pytorch#161317, pytorch#161328, pytorch#161329
mansiag05 pushed a commit to mansiag05/pytorch that referenced this pull request Sep 22, 2025
@swolchok @mansiag05
PythonArgs::toBool: order cheap mutually exclusive checks first (pyto…
c4ce256 
…rch#161455)

symbools are not identical with Py_True or PyFalse, so we can do those cheap checks first and at least get plain old bools to go fast.
Pull Request resolved: pytorch#161455
Approved by: https://github.com/Skylion007
ghstack dependencies: pytorch#161301, pytorch#161292, pytorch#161304, pytorch#161308, pytorch#161315, pytorch#161317, pytorch#161328, pytorch#161329, pytorch#161432
@github-actions github-actions bot deleted the gh/swolchok/798/head branch September 30, 2025 02:08
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

@malfet malfet malfet approved these changes

@bdhirsh bdhirsh bdhirsh approved these changes

@laithsakka laithsakka Awaiting requested review from laithsakka

@wanchaol wanchaol Awaiting requested review from wanchaol

@ezyang ezyang Awaiting requested review from ezyang

@zpcore zpcore Awaiting requested review from zpcore

@XilunWu XilunWu Awaiting requested review from XilunWu

Assignees

No one assigned

Labels

Merged topic: not user facing topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@swolchok @pytorchmergebot @Skylion007 @malfet @bdhirsh