[ONNX] Ignore print(Tensor) during tracing

[thiagocrepaldi]

Fixes #73619
Fixes microsoft/onnxruntime#11812

This PR adds new symbolics: aten::_conj, aten::conj_physical, aten::resolve_conj, and aten::resolve_neg
While the last two are always NO-OP by definition (do not change nodes), the first raises an exception as they are not supported by ONNX yet

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR
• 📄 Preview C++ docs built from this PR
• ❓ Need help or want to give feedback on the CI? Visit our office hours

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

✅ No Failures

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

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: thiagocrepaldi / name: Thiago Crepaldi (486206b, e5ba4e2)

[justinchuby]

I think Bowen has a PR #86180 (some race condition happening 😅 )

[justinchuby]

comment in line

[garymm]

I am not working on PyTorch these days, please don't ask me to review.

[BowenBao]

Closing #86180 since duplicated, let's focus on this one.

For aten::resolve_conj and aten::resolve_neg, pasting previous discussion here

Should we handle when the inputs are complex and we actually want to take the conjugate? I don't remember if this is supported yet. If not maybe a TODO comment?

These two ops are also noops for ONNX complex. It's not taking conjugate, but rather materializing it since torch stores additional conjugate bit. So it's only adjusting the internal representation but not its "value", and from ONNX perspective this op does nothing. Adding to the comment.

• Does aten::_neg and aten::neg_physical exist, similar to aten::_conj and aten::conj_physical?
• aten::resolve_neg & aten::resolve_conj does not alter value, hence even for ONNX complex it can be noop. Unless ONNX also uses conj/neg bit and wish to expose this implementation detail through ONNX operators.
• Please update PR description the new ops supported.
• Please consider adding below as test case too, it's root cause why these complex ops are called in print.

                # 'tolist' has side effect calling 'resolve_conj' and 'resolve_neg'.
                # Annotation added to pass torch script.
                _: List[float] = x.tolist()

[thiagocrepaldi]

Closing #86180 since duplicated, let's focus on this one.

For aten::resolve_conj and aten::resolve_neg, pasting previous discussion here

Should we handle when the inputs are complex and we actually want to take the conjugate? I don't remember if this is supported yet. If not maybe a TODO comment?

These two ops are also noops for ONNX complex. It's not taking conjugate, but rather materializing it since torch stores additional conjugate bit. So it's only adjusting the internal representation but not its "value", and from ONNX perspective this op does nothing. Adding to the comment.

• Does aten::_neg and aten::neg_physical exist, similar to aten::_conj and aten::conj_physical?
• aten::resolve_neg & aten::resolve_conj does not alter value, hence even for ONNX complex it can be noop. Unless ONNX also uses conj/neg bit and wish to expose this implementation detail through ONNX operators.
• Please update PR description the new ops supported.
• Please consider adding below as test case too, it's root cause why these complex ops are called in print.

                # 'tolist' has side effect calling 'resolve_conj' and 'resolve_neg'.
                # Annotation added to pass torch script.
                _: List[float] = x.tolist()

I believe all your comments were addressed. See what you think

[justinchuby]

Suggested change

	self.run_test(
	m,
	x,
	)
	self.run_test(PrintTensorOnMyModel(), x)

for simplicity and consistency

[thiagocrepaldi]

Actually lintrunner f did this. I just left the linter do the linting for me :)

[justinchuby]

That's because there is a comma after x,. Try removing the comma and run it again?

[justinchuby]

Suggested change

m = PrintTensorOnMyModel().eval()

[thiagocrepaldi]

m is needed, otherwise the model is not instantiated

[justinchuby]

For simplicity and consistency with other tests, I would just call PrintTensorOnMyModel() in run test (comment above).

[thiagocrepaldi]

Each test has its own specificities. It is subjective to expect "consistency" in their implementation. The reasoning for keeping the eval was that it came from the bug report

[justinchuby]

Thanks for the context! If a test needs different treatment, I recommend adding a comment on the why to help future readers.

FYI I ran lintrunner locally on the file and got this result

Just for future reference. This is a rather minor detail we can always fix in some other changes in the future. The trick is to remove the trailing comma and let black add them when it sees the need

[justinchuby]

I would expect no errors now? Maybe have separate tests for _conj and conj_physical?

[thiagocrepaldi]

As mentioned on a previous review, an exception is also raised for complex when shape inference is enabled (and it is enabled by default).

[thiagocrepaldi]

No need to create a test that will execute the exact same code as the existing ones. They share implementation

[justinchuby]

I wonder why it errors, is it because of a bug in shape inference? (keep open for reference)

[thiagocrepaldi]

several jit passes do not expect complex numbers. for optimization-only passes, it would be safe to just return from the pass without making any change, preventing the failure. On the other hand, we are not interested in having ONNX graphs without shape inference (IIRC it is not part of the public API). We only use disable it for specific debugging scenarios

[BowenBao]

LGTM

[BowenBao]

Not sure why many seemingly unrelated jobs where failing, I'll try with a rebase.

[BowenBao]

@pytorchbot rebase

[pytorchmergebot]

@pytorchbot successfully started a rebase job. Check the current status here

[pytorchmergebot]

Successfully rebased thiagofc/fix-resolve_conj onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout thiagofc/fix-resolve_conj && git pull --rebase)

[BowenBao]

@thiagocrepaldi the CI errors are legit, the names of new functions in symbolic_opsetx.py needs to be added into __all__.
Noticed it's not the symbolic function ones, but this

2022-10-13T18:01:42.1984484Z # torch.onnx.symbolic_helper.is_complex_value:
2022-10-13T18:01:42.1985010Z   - Is NOT public: it is not inside the module's (`torch.onnx.symbolic_helper`) `__all__`
2022-10-13T18:01:42.1985728Z   - Does look public: it does look public because it follows the rules from the doc above (does not start with `_` and has a proper `__module__`).
2022-10-13T18:01:42.1986309Z   - You can do either of these two things to fix this problem:
2022-10-13T18:01:42.1986844Z     - To make it public: add it from the modules's (`torch.onnx.symbolic_helper`) `__all__`
2022-10-13T18:01:42.1987360Z     - To make it NOT look public: make its name start with `_`

[thiagocrepaldi]

@thiagocrepaldi the CI errors are legit, the names of new functions in symbolic_opsetx.py needs to be added into __all__. Noticed it's not the symbolic function ones, but this

2022-10-13T18:01:42.1984484Z # torch.onnx.symbolic_helper.is_complex_value:
2022-10-13T18:01:42.1985010Z   - Is NOT public: it is not inside the module's (`torch.onnx.symbolic_helper`) `__all__`
2022-10-13T18:01:42.1985728Z   - Does look public: it does look public because it follows the rules from the doc above (does not start with `_` and has a proper `__module__`).
2022-10-13T18:01:42.1986309Z   - You can do either of these two things to fix this problem:
2022-10-13T18:01:42.1986844Z     - To make it public: add it from the modules's (`torch.onnx.symbolic_helper`) `__all__`
2022-10-13T18:01:42.1987360Z     - To make it NOT look public: make its name start with `_`

Symbolics were in __all__

Adding is_complex_value to the symbolic helper's __all__ too

[thiagocrepaldi]

@pytorchbot merge -f "Ignoring unrelated 'Build should have OpenMP enabled, but torch.backends.openmp.is_available() is False'"

[pytorchmergebot]

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

[github-actions]

Hey @thiagocrepaldi.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.