[jit] tracer flag to guard risky behaivors#36277
Closed
wanchaol wants to merge 5 commits intogh/wanchaol/95/basefrom
Closed
[jit] tracer flag to guard risky behaivors#36277wanchaol wants to merge 5 commits intogh/wanchaol/95/basefrom
wanchaol wants to merge 5 commits intogh/wanchaol/95/basefrom
Conversation
This PR introduce a flag to the tracer that guard the risky behaviors like adding list/dict as output of the tracer. Currently to ensure not BC breaking user, we throw warning if the tracer output is list, and will throw error when the tracer output is dict to enforce using this flag (next PR) [ghstack-poisoned]
💊 Build failures summary and remediationsAs of commit 22e5567 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker. This comment has been revised 18 times. |
This PR introduce a flag to the tracer that guard the risky behaviors like adding list/dict as output of the tracer. Currently to ensure not BC breaking user, we throw warning if the tracer output is list, and will throw error when the tracer output is dict to enforce using this flag (next PR) [ghstack-poisoned]
wanchaol
commented
Apr 9, 2020
|
|
||
| trace_graph, torch_out, inputs_states = \ | ||
| torch.jit._get_trace_graph(func, args, _force_outplace=False, _return_inputs_states=True) | ||
| torch.jit._get_trace_graph(func, args, strict=False, _force_outplace=False, _return_inputs_states=True) |
Collaborator
Author
There was a problem hiding this comment.
@houseroad I am relaxing the constraints of onnx exporter to use this strict=False flag, let me know if you see any issues from this.
jamesr66a
reviewed
Apr 9, 2020
Collaborator
jamesr66a
left a comment
There was a problem hiding this comment.
Great, thanks! Just some small comments. Functionality looks good
|
|
||
| return ge | ||
|
|
||
| def checkTracerWarning(self, *args, **kwargs): |
Collaborator
There was a problem hiding this comment.
Instead of having this function, can we use unittest.TestCase.assertWarnsRegex? e.g. all existing callsites can be mechanically replaced like:
self.checkTracerWarning(foo, torch.rand(3, 4), _force_outplace=True)
to
with self.assertWarnsRegex(torch.jit.TracerWarning, "cause the trace to be incorrect""):
torch.jit.trace(foo, torch.rand(3, 4), _force_outplace=True)
This will have the benefit that it's more explicit about what's going on and will force the writer to think about if there is a more specific string they could match on, helping to alleviate the risk of false-positives
| with self.assertRaisesRegex(RuntimeError, r"Only tensors.+can be output from traced functions"): | ||
| traced_f = torch.jit.trace(f, []) | ||
|
|
||
|
|
|
|
||
| def _get_trace_graph(f, args=(), kwargs=None, _force_outplace=False, | ||
| def _get_trace_graph(f, args=(), kwargs=None, strict=True, _force_outplace=False, | ||
| return_inputs=False, _return_inputs_states=False): |
Collaborator
There was a problem hiding this comment.
Nothing actionable on this PR, but we'll have to keep an eye on the proliferation of boolean flags. Four boolean flags means 16 different "modes" the code can go through, and it's unlikely that we're testing all 16. I wonder if we can coalesce some of the other flags into one (e.g. an onnx flag or something)
This PR introduce a flag to the tracer that guard the risky behaviors like adding list/dict as output of the tracer. Currently to ensure not BC breaking user, we throw warning if the tracer output is list, and will throw error when the tracer output is dict to enforce using this flag (next PR) [ghstack-poisoned]
This PR introduce a flag to the tracer that guard the risky behaviors like adding list/dict as output of the tracer. Currently to ensure not BC breaking user, we throw warning if the tracer output is list, and will throw error when the tracer output is dict to enforce using this flag (next PR) [ghstack-poisoned]
This PR introduce a flag to the tracer that guard the risky behaviors like adding list/dict as output of the tracer. Currently to ensure not BC breaking user, we throw warning if the tracer output is list, and will throw error when the tracer output is dict to enforce using this flag (next PR) [ghstack-poisoned]
jamesr66a
approved these changes
Apr 13, 2020
Contributor
laurentdupin
pushed a commit
to laurentdupin/pytorch
that referenced
this pull request
Apr 24, 2026
Summary: Pull Request resolved: pytorch#36277 This PR introduce a flag to the tracer that guard the risky behaviors like adding list/dict as output of the tracer. Currently to ensure not BC breaking user, we throw warning if the tracer output is list, and will throw error when the tracer output is dict to enforce using this flag (next PR) Test Plan: Imported from OSS Differential Revision: D20998157 Pulled By: wanchaol fbshipit-source-id: 0d2c55f1a263a48b1b92dd6ad54407815e0a6f72
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
This PR introduce a flag to the tracer that guard the risky behaviors
like adding list/dict as output of the tracer. Currently to ensure not
BC breaking user, we throw warning if the tracer output is list, and
will throw error when the tracer output is dict to enforce using this
flag (next PR)
Differential Revision: D20998157