[jit] Add dictionary as output of tracer by wanchaol · Pull Request #31860 · pytorch/pytorch

wanchaol · 2020-01-04T18:52:25Z

Stack from ghstack:

[jit] Add dictionary as output of tracer #31860 [jit] Add dictionary as output of tracer
[jit] remove Dict iterationOrder and use insertion order #36609 [jit] add comment for Dict iterationOrder

Summary:

This add dictionary as a supported output of tracer.

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: D20638438

Summary: This add dictionary as a supported output of tracer. Current issue: StringType is not handled in getOutput, so it will error out Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: b62934e Pull Request resolved: #31860

kostmo · 2020-01-04T19:18:51Z

💊 Build failures summary and remediations

As of commit 89ad086 (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.

See how this bot performed.

This comment has been revised 39 times.

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: eade920 Pull Request resolved: #31860

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: c866a16 Pull Request resolved: #31860

…nistic

bddppq · 2020-04-03T05:24:31Z

The surprising behavior demonstrated in #31860 (review) is actually not caused by the limitation of static dict, but rather the size Tensor has been casted to Python int in the range(x.size(0)) expression. With other container types tracing has similar behavior:

import torch

def foo_list(x):
  rv = []
  for i in range(x.size(0)):
    rv.append(x[i])
  return rv

traced = torch.jit.trace(foo_list, (torch.randn(3, 1)))
print(traced(torch.randn(3, 1)))
print(traced(torch.randn(5, 1)))

def foo_tuple(x):
    return tuple(x[i] for i in range(x.size(0)))

traced = torch.jit.trace(foo_tuple, (torch.randn(3, 1)))
print(traced(torch.randn(3, 1)))
print(traced(torch.randn(5, 1)))

 TracerWarning: Converting a tensor to a Python index might cause the trace to be incorrect. We can't record the data flow of P
ython values, so this value will be treated as a constant in the future. This means that the trace might not generalize to other inputs!
  for i in range(x.size(0)):
[tensor([-0.0323]), tensor([-2.0990]), tensor([0.3493])]
[tensor([-0.3254]), tensor([1.0486]), tensor([-0.7183])]
TracerWarning: Converting a tensor to a Python index might cause the trace to be incorrect. We can't record the data flow of
Python values, so this value will be treated as a constant in the future. This means that the trace might not generalize to other inputs!
  return tuple(x[i] for i in range(x.size(0)))
(tensor([0.5241]), tensor([-0.1070]), tensor([-2.0607]))
(tensor([-1.0013]), tensor([1.5692]), tensor([-0.1939]))

bddppq · 2020-04-03T05:59:44Z

So if we are going to add a flag to prevent this silently wrong behavior, maybe it should be guarding the casting (but it will be a breaking change, right now the tracer only gives warning).

wanchaol · 2020-04-03T06:09:06Z

So if we are going to add a flag to prevent this silently wrong behavior, maybe it should be guarding the casting (but it will be a breaking change, right now the tracer only gives warning).

@bddppq you are right, the case only happens in dynamic control flow, and casting currently gives a warning to the user that might get ignored by user who don't read the doc and understand the internals of tracer. The general issue might not only happening for tensor casting though, i.e. user could check len(list) or len(dict) inside the tracer in the control flow and do whatever they want, but a changed input might be wrong there too.

Had a discussion with @jamesr66a and other folks today. The short term plan is to guard the risk behavior under a strict flag, and throw warnings/errors to user. Right now tensor casting will throw warning, others not. To ensure not BC breaking with existing user code too much, we should probably:

throw warning in tracer when discovering list (input/output) + dict(input), these are thing already supported but no warning yet.
for dict output, we will throw error and explicitly guide the user to use strict=False flag.

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20638438](https://our.internmc.facebook.com/intern/diff/D20638438) [ghstack-poisoned]

Summary: This add dictionary as a supported output of tracer. it will throw error to user by default if they want to use it, and only allow this when strict=False when call `torch.jit.trace` Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: a574e9f Pull Request resolved: #31860

jamesr66a

Cool, I had a question about how we're doing ordering for Tensor-keyed dictionaries. I want to make sure that's safe and stable across compilation/machines etc

jamesr66a · 2020-04-09T18:27:38Z

+    TypePtr key_type = dict.keyType();
+    TypePtr value_type = dict.valueType();
+
+    if (!(key_type->cast<StringType>() || key_type->cast<TensorType>()) ||


This is kinda hard to read in the !(A || B) || !C form. Can we do something like:

bool key_type_valid = key_type->cast<StringType>() || key_type->cast<TensorType>(); bool value_type_valid = value_type->cast<TensorType>(); if (!key_type_valid || !value_type_valid) { <...>

jamesr66a · 2020-04-09T18:29:54Z

+         << "or dict[Tensor, Tensor] can be a dictionary output of a traced function";
+      throw std::runtime_error(os.str());
+    }
+    const auto order = iterationOrder(dict);


Not an issue with your PR, but I looked at the declaration of iterationOrder and I'm not sure what it does :p. It could use some comments or a rename to indicate its function

iterationOrder will return a pair of ivalues that ordered by CompareKeys, in the case that key is Tensor, it's ordered by TensorImpl pointer address.

Can you (or someone) add that as a comment?

did this in #36609

jamesr66a · 2020-04-09T18:32:55Z

+      std::iota(perm.begin(), perm.end(), 0);
+      std::sort(perm.begin(), perm.end(), [&](int64_t i, int64_t j) {
+        return keys[i]->unique() < keys[j]->unique();
+      });


Can you comment on the stability of these unique IDs? Doesn't Value::unique() potentially depend on the order of compilation of values?

I think this is quite stable, by the time we generate the output, the tracer already record the values of the dict keys, and the tracer record the keys based on the order of encountering tensors, so unique() should depend on the order of that, and tracing the same program will always generate the same sequence of unique ids.

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20638438](https://our.internmc.facebook.com/intern/diff/D20638438) [ghstack-poisoned]

Summary: This add dictionary as a supported output of tracer. it will throw error to user by default if they want to use it, and only allow this when strict=False when call `torch.jit.trace` Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 3081ce8 Pull Request resolved: #31860

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20638438](https://our.internmc.facebook.com/intern/diff/D20638438) [ghstack-poisoned]

Summary: This add dictionary as a supported output of tracer. it will throw error to user by default if they want to use it, and only allow this when strict=False when call `torch.jit.trace` Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 29a3010 Pull Request resolved: #31860

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20638438](https://our.internmc.facebook.com/intern/diff/D20638438) [ghstack-poisoned]

Summary: This add dictionary as a supported output of tracer. it will throw error to user by default if they want to use it, and only allow this when strict=False when call `torch.jit.trace` Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 026b826 Pull Request resolved: #31860

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20638438](https://our.internmc.facebook.com/intern/diff/D20638438) [ghstack-poisoned]

Summary: This add dictionary as a supported output of tracer. it will throw error to user by default if they want to use it, and only allow this when strict=False when call `torch.jit.trace` Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 83af02c Pull Request resolved: #31860

wanchaol · 2020-04-16T00:57:18Z

something weird happens with ghstack on this PR, replace this with #36696

wanchaol requested a review from apaszke as a code owner January 4, 2020 18:52

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jan 4, 2020

wanchaol changed the title ~~[WIP] Add dictionary as output of tracer~~ [jit] Add dictionary as output of tracer Mar 3, 2020

wanchaol added a commit that referenced this pull request Mar 3, 2020

[jit] Add dictionary as output of tracer

3979fea

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: eade920 Pull Request resolved: #31860

wanchaol added a commit that referenced this pull request Mar 5, 2020

[jit] Add dictionary as output of tracer

7c433e5

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: c866a16 Pull Request resolved: #31860

wanchaol requested review from aazzolini, albanD, ebetica, ezyang, fmassa, goldsborough, mrshenli, pietern, pritamdamania87, soumith, yf225 and zhaojuanmao as code owners March 24, 2020 10:47

bddppq force-pushed the gh/wanchaol/75/head branch from 0cf4e6f to 9938cc8 Compare March 24, 2020 10:49

[jit] Add dictionary as output of tracer

9f81e8d

bddppq force-pushed the gh/wanchaol/75/head branch from 9938cc8 to 9f81e8d Compare March 24, 2020 10:57

bddppq added 2 commits March 24, 2020 20:18

Sort Tensor keys by their unique ids to make the traced graph determi…

c68f2dc

…nistic

Merge branch 'gh/wanchaol/75/base' into gh/wanchaol/75/head

83641a6

wanchaol requested review from suo and removed request for ebetica, ezyang, pietern, soumith and yf225 March 24, 2020 22:27

wanchaol mentioned this pull request Apr 8, 2020

[jit] tracer flag to guard risky behaivors #36277

Closed

Update on "[jit] Add dictionary as output of tracer"

7d9445a

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20638438](https://our.internmc.facebook.com/intern/diff/D20638438) [ghstack-poisoned]

Update on "[jit] Add dictionary as output of tracer"

268b6da

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20638438](https://our.internmc.facebook.com/intern/diff/D20638438) [ghstack-poisoned]

Update on "[jit] Add dictionary as output of tracer"

d69d2af

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20638438](https://our.internmc.facebook.com/intern/diff/D20638438) [ghstack-poisoned]

wanchaol requested a review from jamesr66a April 8, 2020 22:57

Update on "[jit] Add dictionary as output of tracer"

3be72f9

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20638438](https://our.internmc.facebook.com/intern/diff/D20638438) [ghstack-poisoned]

jamesr66a reviewed Apr 9, 2020

View reviewed changes

wanchaol mentioned this pull request Apr 10, 2020

Use unittest assertWarns instead #36411

Closed

Update on "[jit] Add dictionary as output of tracer"

0686a14

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20638438](https://our.internmc.facebook.com/intern/diff/D20638438) [ghstack-poisoned]

Update on "[jit] Add dictionary as output of tracer"

1f84069

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20638438](https://our.internmc.facebook.com/intern/diff/D20638438) [ghstack-poisoned]

wanchaol requested a review from jamesr66a April 13, 2020 17:05

Update on "[jit] Add dictionary as output of tracer"

c8fad91

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20638438](https://our.internmc.facebook.com/intern/diff/D20638438) [ghstack-poisoned]

Update on "[jit] Add dictionary as output of tracer"

89ad086

Summary: This add dictionary as a supported output of tracer. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20638438](https://our.internmc.facebook.com/intern/diff/D20638438) [ghstack-poisoned]

wanchaol mentioned this pull request Apr 14, 2020

[jit] remove Dict iterationOrder and use insertion order #36609

Closed

wanchaol merged commit 4d2da2e into gh/wanchaol/75/base Apr 16, 2020

jluntamazon mentioned this pull request Sep 29, 2021

How to compile pre-trained MaskRCNN PyTorch model on AWS Neuron Compiler? aws-neuron/aws-neuron-sdk#331

Closed

fxmarty mentioned this pull request Oct 28, 2022

Add FXModel huggingface/optimum#439

Closed

3 tasks

Conversation

wanchaol commented Jan 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kostmo commented Jan 4, 2020 • edited by dr-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 Build failures summary and remediations

Uh oh!

bddppq commented Apr 3, 2020

Uh oh!

bddppq commented Apr 3, 2020

Uh oh!

wanchaol commented Apr 3, 2020

Uh oh!

jamesr66a left a comment

Choose a reason for hiding this comment

Uh oh!

jamesr66a Apr 9, 2020

Choose a reason for hiding this comment

Uh oh!

jamesr66a Apr 9, 2020

Choose a reason for hiding this comment

Uh oh!

wanchaol Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

jamesr66a Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

wanchaol Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

jamesr66a Apr 9, 2020

Choose a reason for hiding this comment

Uh oh!

wanchaol Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

wanchaol commented Apr 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wanchaol commented Jan 4, 2020 •

edited

Loading

kostmo commented Jan 4, 2020 •

edited by dr-ci Bot

Loading