Skip to content

[ONNX] Remove unused patching methods#82511

Closed
justinchuby wants to merge 16 commits intopytorch:masterfrom
justinchuby:justinchu/patch
Closed

[ONNX] Remove unused patching methods#82511
justinchuby wants to merge 16 commits intopytorch:masterfrom
justinchuby:justinchu/patch

Conversation

@justinchuby
Copy link
Collaborator

@justinchuby justinchuby commented Jul 29, 2022 •
edited
Loading

Description

Remove unused patching methods:

  • torch._C.Graph.constant
  • unpatch torch._C.Node.__getitem__ and move the helper function to symbolic_helper

Add typing annotations

Issue

Fixes #76254
Dependent on #81953, #82628

Testing

Unit tested

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 29, 2022 •
edited
Loading

🔗 Helpful links

❌ 1 New Failures

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

Expand to see more
  • 1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages

See GitHub Actions build pull / linux-focal-py3.7-clang10-onnx / test (default, 1, 2, linux.2xlarge) (1/1)

Step: "Test" (full log | diagnosis details)

2022-08-05T23:40:28.4575046Z TypeError: 'torch._C.Node' object is not subscriptable 
2022-08-05T23:40:28.4569094Z   File "/opt/conda/lib/python3.7/site-packages/torch/onnx/utils.py", line 1422, in _export
2022-08-05T23:40:28.4569563Z     dynamic_axes=dynamic_axes,
2022-08-05T23:40:28.4570217Z   File "/opt/conda/lib/python3.7/site-packages/torch/onnx/utils.py", line 1063, in _model_to_graph
2022-08-05T23:40:28.4570669Z     module=module,
2022-08-05T23:40:28.4571262Z   File "/opt/conda/lib/python3.7/site-packages/torch/onnx/utils.py", line 625, in _optimize_graph
2022-08-05T23:40:28.4572034Z     graph = _C._jit_pass_onnx(graph, operator_export_type)
2022-08-05T23:40:28.4572760Z   File "/opt/conda/lib/python3.7/site-packages/torch/onnx/utils.py", line 1745, in _run_symbolic_function
2022-08-05T23:40:28.4573285Z     return symbolic_fn(g, *inputs, **attrs)
2022-08-05T23:40:28.4573992Z   File "/opt/conda/lib/python3.7/site-packages/torch/onnx/symbolic_caffe2.py", line 184, in upsample_nearest2d
2022-08-05T23:40:28.4574506Z     "Y_scale_f": input.node()["Y_scale"],
2022-08-05T23:40:28.4575046Z TypeError: 'torch._C.Node' object is not subscriptable 
2022-08-05T23:40:28.4575503Z (Occurred when translating upsample_nearest2d).
2022-08-05T23:40:28.4575772Z 
2022-08-05T23:40:28.4575780Z 
2022-08-05T23:40:28.4575993Z 
2022-08-05T23:40:28.4576173Z 
2022-08-05T23:40:28.4594775Z [gw2] �[31mFAILED�[0m test/onnx/test_pytorch_onnx_caffe2_quantized.py �[1A
2022-08-05T23:40:28.5200927Z  �[36mtest/onnx/test_pytorch_onnx_caffe2_quantized.py�[0m::TestQuantizedOps.test_upsample�[0m �[31m⨯�[0m�[31m96% �[0m�[40m�[32m█�[0m�[40m�[32m█�[0m�[40m�[32m█�[0m�[40m�[32m█�[0m�[40m�[32m█�[0m�[40m�[32m█�[0m�[40m�[32m█�[0m�[40m�[32m█�[0m�[40m�[32m█�[0m�[40m�[31m▋�[0m�[1B
2022-08-05T23:40:28.5201426Z 
2022-08-05T23:40:28.5203306Z 
2022-08-05T23:40:28.5203466Z
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.

@justinchuby justinchuby changed the title [ONNX] Clean up patch [ONNX] Remove unused patching methods Jul 30, 2022
@justinchuby justinchuby marked this pull request as ready for review July 30, 2022 00:09
@justinchuby justinchuby added the module: onnx Related to torch.onnx label Jul 30, 2022
@titaiwangms
Copy link
Collaborator

titaiwangms commented Aug 1, 2022

I think the changes are good, but this is failing a lot of tests. Please make sure it isn't about the PR.

@justinchuby
Copy link
Collaborator Author

justinchuby commented Aug 1, 2022

Thanks - fixing errors

@justinchuby
Copy link
Collaborator Author

justinchuby commented Aug 1, 2022

Updated. _type_utils is dependent on #81953 which needs reviews

@justinchuby justinchuby added the topic: improvements topic category label Aug 1, 2022
@justinchuby justinchuby mentioned this pull request Aug 2, 2022
Closed
pytorchmergebot pushed a commit that referenced this pull request Aug 2, 2022
@justinchuby @pytorchmergebot
[tensorboard] Remove dependence on torch.onnx (#82628)
c7dde7d 
### Description
<!-- What did you change and why was it needed? -->

The `graph` function takes a dependency on `torch.onnx.select_model_mode_for_export`, which was used because it implicitly patches `torch._C.Node` to allow for key access. This change removed the need for the patch and decoupled the tensorboard util from `torch.onnx`.

This is needed to unblock #82511 because we are removing the monkey patch

### Issue
<!-- Link to Issue ticket or RFP -->

### Testing
<!-- How did you test your change? -->

cc @ezyang @orionr @BowenBao
Pull Request resolved: #82628
Approved by: https://github.com/ezyang
facebook-github-bot pushed a commit that referenced this pull request Aug 3, 2022
@justinchuby @facebook-github-bot
[tensorboard] Remove dependence on torch.onnx (#82628) (#82628)
cec8deb 
Summary:
### Description
<!-- What did you change and why was it needed? -->

The `graph` function takes a dependency on `torch.onnx.select_model_mode_for_export`, which was used because it implicitly patches `torch._C.Node` to allow for key access. This change removed the need for the patch and decoupled the tensorboard util from `torch.onnx`.

This is needed to unblock #82511 because we are removing the monkey patch

### Issue
<!-- Link to Issue ticket or RFP -->

### Testing
<!-- How did you test your change? -->

cc ezyang orionr BowenBao

Pull Request resolved: #82628
Approved by: https://github.com/ezyang

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

Reviewed By: kit1980

Differential Revision: D38359651

fbshipit-source-id: d2c3e4f0db75b54d224e11c76830ca642edaa2e9
@albanD albanD added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 3, 2022
@BowenBao
Copy link
Collaborator

BowenBao commented Aug 3, 2022

Updated. _type_utils is dependent on #81953 which needs reviews

ghstack would come in handy. It would be nicer if you could split the annotation part from the patching change.

@justinchuby
Copy link
Collaborator Author

justinchuby commented Aug 3, 2022

Updated. _type_utils is dependent on #81953 which needs reviews

ghstack would come in handy. It would be nicer if you could split the annotation part from the patching change.

Thanks, I will use ghstack in the next batch of PRs.

@titaiwangms
Copy link
Collaborator

titaiwangms commented Aug 8, 2022

Seems there are complaints from quantization UTs.

@justinchuby justinchuby marked this pull request as draft August 8, 2022 17:03
@titaiwangms
Copy link
Collaborator

titaiwangms commented Aug 8, 2022

Seems there are complaints from quantization UTs.

Could you also update why you had and how you fix those failed checks after you manage to fix them? I am curious.

@justinchuby
Copy link
Collaborator Author

justinchuby commented Aug 8, 2022

Closed in favor of #83006

@justinchuby justinchuby closed this Aug 8, 2022
jamt9000 added a commit to unitaryai/clip-torch2 that referenced this pull request Jul 6, 2023
@jamt9000
Fix torch._C.Node attribute access
0e54df1 
Attribute access with subscripting would previously work
due to patching in pytorch/pytorch#82511
but this has been removed.

This commit uses the fix proposed in pytorch/pytorch#82628
to define a helper method to call the appropriate access method.
@jamt9000 jamt9000 mentioned this pull request Jul 6, 2023
Merged
jamt9000 added a commit to unitaryai/clip-torch2 that referenced this pull request Jul 6, 2023
@jamt9000
Fix torch._C.Node attribute access
2dbac90 
Attribute access with subscripting would previously work
due to patching in pytorch/pytorch#82511
but this has been removed.

This commit uses the fix proposed in pytorch/pytorch#82628
to define a helper method to call the appropriate access method.
jongwook pushed a commit to openai/CLIP that referenced this pull request Jul 8, 2023
@jamt9000
Fix torch._C.Node attribute access (#372)
a1d0717 
Attribute access with subscripting would previously work
due to patching in pytorch/pytorch#82511
but this has been removed.

This commit uses the fix proposed in pytorch/pytorch#82628
to define a helper method to call the appropriate access method.
grandcyw pushed a commit to grandcyw/CLIP that referenced this pull request Nov 21, 2023
@jamt9000 @grandcyw
Fix torch._C.Node attribute access (openai#372)
8478670 
Attribute access with subscripting would previously work
due to patching in pytorch/pytorch#82511
but this has been removed.

This commit uses the fix proposed in pytorch/pytorch#82628
to define a helper method to call the appropriate access method.
grandcyw added a commit to grandcyw/CLIP that referenced this pull request Nov 21, 2023
@grandcyw
Fix torch._C.Node attribute access (openai#372)
f6e4bbf 
Attribute access with subscripting would previously work
due to patching in pytorch/pytorch#82511
but this has been removed.

This commit uses the fix proposed in pytorch/pytorch#82628
to define a helper method to call the appropriate access method.
grandcyw added a commit to grandcyw/CLIP that referenced this pull request Nov 21, 2023
@grandcyw
Fix torch._C.Node attribute access (openai#372)
c0d58be 
Attribute access with subscripting would previously work
due to patching in pytorch/pytorch#82511
but this has been removed.

This commit uses the fix proposed in pytorch/pytorch#82628
to define a helper method to call the appropriate access method.
Jung-Hoon-Sung pushed a commit to Jung-Hoon-Sung/Drone-images-finetuning-clip that referenced this pull request Jan 8, 2024
@jamt9000
Fix torch._C.Node attribute access (#372)
61a6b09 
Attribute access with subscripting would previously work
due to patching in pytorch/pytorch#82511
but this has been removed.

This commit uses the fix proposed in pytorch/pytorch#82628
to define a helper method to call the appropriate access method.
rom1504 pushed a commit to rom1504/CLIP that referenced this pull request Jan 13, 2024
@jamt9000 @rom1504
Fix torch._C.Node attribute access (openai#372)
62e1851 
Attribute access with subscripting would previously work
due to patching in pytorch/pytorch#82511
but this has been removed.

This commit uses the fix proposed in pytorch/pytorch#82628
to define a helper method to call the appropriate access method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BowenBao BowenBao Awaiting requested review from BowenBao

@shubhambhokare1 shubhambhokare1 Awaiting requested review from shubhambhokare1

Assignees

No one assigned

Labels

cla signed module: onnx Related to torch.onnx open source topic: improvements topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ONNX] Remove the function _graph_constant in utils if not needed

6 participants

@justinchuby @facebook-github-bot @titaiwangms @BowenBao @albanD @pytorchbot