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

typing proxy_tensor.py #129182

Closed
wants to merge 17 commits into from
Closed

typing proxy_tensor.py #129182

wants to merge 17 commits into from

Conversation

aorenste
Copy link
Contributor

@aorenste aorenste commented Jun 20, 2024 •
edited
Loading

@aorenste
typing proxy_tensor.py
18597df 
[ghstack-poisoned]
@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Jun 20, 2024 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

As of commit 61d095d with merge base d443fbc (image):
💚 Looks good so far! There are no failures yet. 💚

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

@pytorch-bot pytorch-bot bot added ciflow/inductor module: dynamo module: inductor oncall: distributed Add this issue/PR to distributed oncall triage queue release notes: fx release notes category labels Jun 20, 2024
@aorenste
Update on "typing proxy_tensor.py"
54a7679 
cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k voznesenskym EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire

[ghstack-poisoned]
@aorenste aorenste mentioned this pull request Jun 21, 2024
Closed
@aorenste
Update on "typing proxy_tensor.py"
150eeff 
cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k voznesenskym EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire

[ghstack-poisoned]
@aorenste
Update on "typing proxy_tensor.py"
6e58dfa 
cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k voznesenskym EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire

[ghstack-poisoned]
@aorenste
Update on "typing proxy_tensor.py"
ae96fc5 
cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k voznesenskym EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire

[ghstack-poisoned]
@aorenste
Update on "typing proxy_tensor.py"
acb1297 
cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k voznesenskym EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire

[ghstack-poisoned]
@aorenste aorenste mentioned this pull request Jun 22, 2024
Closed
@aorenste
Update on "typing proxy_tensor.py"
8e6dc62 
cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k voznesenskym EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire

[ghstack-poisoned]
@aorenste
Update on "typing proxy_tensor.py"
f087b33 
cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k voznesenskym EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire

[ghstack-poisoned]
This was referenced Jun 23, 2024
Closed
Closed
@aorenste
Update on "typing proxy_tensor.py"
a817a4b 
cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k voznesenskym EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire

[ghstack-poisoned]
@aorenste aorenste mentioned this pull request Jun 24, 2024
Closed
@aorenste aorenste marked this pull request as ready for review June 24, 2024 16:07
@aorenste
Update on "typing proxy_tensor.py"
02459a1 
cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k voznesenskym EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire

[ghstack-poisoned]
@aorenste aorenste mentioned this pull request Jun 25, 2024
Closed
@aorenste
Update on "typing proxy_tensor.py"
ff357ca 
cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k voznesenskym EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire

[ghstack-poisoned]
@aorenste aorenste mentioned this pull request Jun 28, 2024
Closed
@aorenste
Update on "typing proxy_tensor.py"
0a293dd 
cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k voznesenskym EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire

[ghstack-poisoned]
@aorenste aorenste requested a review from Skylion007 July 2, 2024 18:46
@aorenste
Update on "typing proxy_tensor.py"
845f3f1 
cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k voznesenskym EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire

[ghstack-poisoned]
Chillee
Chillee approved these changes Jul 10, 2024
View reviewed changes
Copy link
Contributor

@Chillee Chillee left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me (although it's a lot of changes...). Main thing is I think some of the overload definitions in proxy_tensor.py are pretty unwieldy.

torch/fx/experimental/proxy_tensor.py
tracer: _ProxyTracer,
default: U,
transform: Callable[[_PySymProxyType], R]
) -> Union[R, U]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how I feel about these overload definitions lol. Wdyt @ezyang

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that they're pretty unweildy - but that's the behavior they have today. It was pretty confusing trying to understand what those functions were doing when the parameter types changed the return type ("Wait - in this case I have to call the returned value? But this one I don't?"). At least now it's (somewhat) documented.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my experience, writing lots of overloads for accurate types is unavoidable. It is mostly a matter of taste and I am willing to defer to @aorenste. I haven't carefully looked at the diff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we type this more concisely with T = TypeVar('T', bound=Union[Tensor, _AnyScriptObjectType, PySymType])

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 more concise, yes. But it's also less specific. The problem is that this function is really like 6 different functions overloaded into a single API - the output type depends on the input type (but indirectly - so not just like 'T'). So we could weaken the typing of the input but that would also weaken the typing of the output.

Copy link
Contributor

Choose a reason for hiding this comment

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

With parameterized type, isn't the output assumed to be the same type as input ? or am i mistaking something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're not mistaken - but that's not what these functions return. For example one version takes _AnyScriptObjectType and returns Proxy - different types.

@aorenste
Update on "typing proxy_tensor.py"
86a48b4 
cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k voznesenskym EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire

[ghstack-poisoned]
@mikaylagawarecki mikaylagawarecki removed their request for review July 10, 2024 18:17
@aorenste
Update on "typing proxy_tensor.py"
fc26d66 
cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k voznesenskym EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire

[ghstack-poisoned]
@aorenste aorenste added suppress-bc-linter Suppresses the failures of API backward-compatibility linter (Lint/bc_linter) topic: not user facing topic category labels Jul 11, 2024
@aorenste
Update on "typing proxy_tensor.py"
61d095d 
cc XilunWu H-Huang awgu kwen2501 wanchaol fegin fduwjj wz337 wconstab d4l3k c-p-i-o voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire chauhang mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse tianyu-l

[ghstack-poisoned]
@aorenste
Copy link
Contributor Author

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jul 12, 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

pytorchmergebot pushed a commit that referenced this pull request Jul 13, 2024
@aorenste @pytorchmergebot
typing fake_tensor.py (#128041)
5674829 
Pull Request resolved: #128041
Approved by: https://github.com/eellison
ghstack dependencies: #129182
@henrylhtsang henrylhtsang mentioned this pull request Jul 17, 2024
Closed
francograndegmailcom pushed a commit to francograndegmailcom/pytorch-pytorch that referenced this pull request Jul 23, 2024
@aorenste
typing proxy_tensor.py
d5b5554 
ghstack-source-id: 2c4bd5c02b098a99f8d9ba6b0407cb336a7f8002
Pull Request resolved: pytorch/pytorch#129182
xuhancn pushed a commit to xuhancn/pytorch that referenced this pull request Jul 25, 2024
@aorenste @xuhancn
typing proxy_tensor.py (pytorch#129182)
3dd6c07 
Pull Request resolved: pytorch#129182
Approved by: https://github.com/Chillee
xuhancn pushed a commit to xuhancn/pytorch that referenced this pull request Jul 25, 2024
@aorenste @xuhancn
typing fake_tensor.py (pytorch#128041)
c7b500c 
Pull Request resolved: pytorch#128041
Approved by: https://github.com/eellison
ghstack dependencies: pytorch#129182
@davidberard98 davidberard98 mentioned this pull request Aug 2, 2024
Closed
@github-actions github-actions bot deleted the gh/aorenste/74/head branch August 12, 2024 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ezyang ezyang ezyang left review comments

@justinchuby justinchuby justinchuby left review comments

@eellison eellison eellison left review comments

@Chillee Chillee Chillee approved these changes

@titaiwangms titaiwangms Awaiting requested review from titaiwangms

@shubhambhokare1 shubhambhokare1 Awaiting requested review from shubhambhokare1

@wschin wschin Awaiting requested review from wschin

@jbschlosser jbschlosser Awaiting requested review from jbschlosser

@Skylion007 Skylion007 Awaiting requested review from Skylion007

Assignees
No one assigned
Labels
ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: dynamo module: inductor oncall: distributed Add this issue/PR to distributed oncall triage queue release notes: fx release notes category suppress-bc-linter Suppresses the failures of API backward-compatibility linter (Lint/bc_linter) topic: not user facing topic category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@aorenste @eellison @Chillee @pytorchmergebot @ezyang @Skylion007 @justinchuby