Skip to content

Conversation

@justinchuby
Copy link
Collaborator

@justinchuby justinchuby commented Oct 26, 2022

Deprecate torch.onnx.operators because it's only for backwards compatibility

@pytorch-bot pytorch-bot bot added the release notes: onnx torch.onnx related changes that should show up in the release notes label Oct 26, 2022
@pytorch-bot
Copy link

pytorch-bot bot commented Oct 26, 2022

🔗 Helpful Links

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

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

❌ 2 Failures

As of commit 7f12f2e:

The following jobs have failed:

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

@justinchuby justinchuby added module: onnx Related to torch.onnx topic: deprecation topic category release notes: onnx torch.onnx related changes that should show up in the release notes and removed release notes: onnx torch.onnx related changes that should show up in the release notes labels Oct 26, 2022
@justinchuby justinchuby changed the title [ONNX] Clean up deprecated operators.py [ONNX] Remove deprecated operators.py Oct 26, 2022
Copy link
Collaborator

@BowenBao BowenBao left a comment

Choose a reason for hiding this comment

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

hmm is this not bc breaking?

@justinchuby justinchuby added the topic: bc breaking topic category label Oct 26, 2022
@justinchuby
Copy link
Collaborator Author

Looks like its used somewhere else. Taking a closer look

@justinchuby
Copy link
Collaborator Author

Hmm. Looks like its still used in caffe2 models. I will add a deprecation message instead?

Copy link
Collaborator

@BowenBao BowenBao left a comment

Choose a reason for hiding this comment

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

lgtm

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 26, 2022
@justinchuby
Copy link
Collaborator Author

@pytorchbot merge -g

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks on your PR pass since you used the green (-g) flag (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
Copy link
Collaborator

Merge failed

Reason: The following mandatory check(s) failed (Rule ONNX exporter):

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

@justinchuby justinchuby changed the title [ONNX] Remove deprecated operators.py [ONNX] Deprecate operators.py Oct 26, 2022
@pytorch-bot
Copy link

pytorch-bot bot commented Oct 26, 2022

❌ 🤖 pytorchbot command failed:

@pytorchbot: error: argument command: invalid choice: 'All pull tests passed; trunk flaky' (choose from 'merge', 'revert', 'rebase', 'label')

usage: @pytorchbot [-h] {merge,revert,rebase,label} ...

Try @pytorchbot --help for more info.

@justinchuby
Copy link
Collaborator Author

@pytorchbot merge -f "All pull tests passed; trunk flaky"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

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

@justinchuby justinchuby deleted the justinchu/remove-operators branch October 26, 2022 22:19
@weiwangmeta
Copy link
Contributor

@pytorchbot revert -m "breaking internal builds see D40797126" -c ghfirst

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

@justinchuby your PR has been successfully reverted.

pytorchmergebot added a commit that referenced this pull request Oct 29, 2022
This reverts commit 88eff10.

Reverted #87798 on behalf of https://github.com/weiwangmeta due to breaking internal builds see D40797126
@justinchuby
Copy link
Collaborator Author

@weiwangmeta could you post the errors from D40797126? Thanks!

@weiwangmeta
Copy link
Contributor

weiwangmeta commented Nov 1, 2022

@weiwangmeta could you post the errors from D40797126? Thanks!

Sorry for the delayed response. The errors stemmed from
https://github.com/facebookresearch/fairseq/blob/main/fairseq/modules/sinusoidal_positional_embedding.py#L68
and it was like

RuntimeError: 
undefined value torch:
...
@_deprecation.deprecated("1.14", "1.16", "use torch._shape_as_tensor")
def shape_as_tensor(x):
    return torch._shape_as_tensor(x)
           ~~~ <--- HERE
'shape_as_tensor' is being compiled since it was called from 'SinusoidalPositionalEmbedding.forward'
  File "....../fairseq/modules/sinusoidal_positional_embedding.py", line 68
    ):
        """Input is expected to be of size [bsz x seqlen]."""
        bspair = torch.onnx.operators.shape_as_tensor(input)
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ <--- HERE
        bsz, seq_len = bspair[0], bspair[1]
        max_pos = self.padding_idx + 1 + seq_len

Stack trace:
Exception type: torch::jit::ErrorReport

@weiwangmeta
Copy link
Contributor

I realize this PR is all about BC breaking and it is not surprising to see the above failures. But looks like we have internal usage that is still relying on the old API.

cc @osalpekar @mehtanirav @seemethere How should we embrace this deprecation?

@justinchuby
Copy link
Collaborator Author

I wonder why torch would be an undefined value? Is this running jit.script?

@weiwangmeta
Copy link
Contributor

weiwangmeta commented Nov 1, 2022

@justinchuby
Copy link
Collaborator Author

Thanks. It could be that jit script doesn't like the deprecation logic. I'd be curious to hear suggestions on the deprecation path, as the underlying aten ops seem to be non-public.

@weiwangmeta
Copy link
Contributor

weiwangmeta commented Nov 2, 2022

Thanks @justinchuby. Let's involve more folks. cc @malfet @kit1980

kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Nov 5, 2022
Deprecate `torch.onnx.operators` because it's only for backwards compatibility
Pull Request resolved: pytorch#87798
Approved by: https://github.com/BowenBao
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Nov 5, 2022
This reverts commit 88eff10.

Reverted pytorch#87798 on behalf of https://github.com/weiwangmeta due to breaking internal builds see D40797126
@malfet
Copy link
Contributor

malfet commented Nov 28, 2022

I see there are two slightly different problem:

  • torch.jit.script fails to recognize the deprecation decorator
  • We should not deprecate public function in favor of private function. May be creating a public wrapper in torch. module is a way to go, but then it needs further discussion (i.e. why was it private in the first place?)

kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Dec 10, 2022
Deprecate `torch.onnx.operators` because it's only for backwards compatibility
Pull Request resolved: pytorch#87798
Approved by: https://github.com/BowenBao
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Dec 10, 2022
This reverts commit 88eff10.

Reverted pytorch#87798 on behalf of https://github.com/weiwangmeta due to breaking internal builds see D40797126
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged module: onnx Related to torch.onnx open source release notes: onnx torch.onnx related changes that should show up in the release notes Reverted topic: bc breaking topic category topic: deprecation topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants