Skip to content

Update createBundle to set symbol input and output #2674

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

gcatron
Copy link
Contributor

@gcatron gcatron commented Apr 8, 2019

Description: This updates the create method on RuntimeBundle to properly set input and output status for symbols.
Testing: ninja test:
Documentation:
Fixes #1953

@gcatron gcatron force-pushed the set_symbol_input_output_runtime_bundle branch 6 times, most recently from 30913db to 9a4ee52 Compare April 12, 2019 17:12
Copy link
Contributor

@SplitInfinity SplitInfinity left a comment

Choose a reason for hiding this comment

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

Let's also consider trainable Placeholders. I think this code will mark them as both inputs and outputs. Is that desired? If so, let's add a unit test for that too.

@nadavrot
Copy link
Contributor

Consider the two scenarios: 1. We have a bug in this code that the unit test catches. 2. We need to change the design of the function to make PH an argument list.

Is this unit test carrying its weight? It will make it difficult for us to change the design of the compiler and at the same time won’t provide much safety. Maybe we can update an existing test instead of adding a new test that we will need to update later?

@gcatron gcatron force-pushed the set_symbol_input_output_runtime_bundle branch from 9a4ee52 to d9c1e13 Compare April 15, 2019 15:12
@nadavrot
Copy link
Contributor

There is a bug here. Save nodes accept placeholders as inputs. If this is a save node you need to check if this is the destination.

@gcatron gcatron force-pushed the set_symbol_input_output_runtime_bundle branch 3 times, most recently from 9f83509 to 745e8e6 Compare April 15, 2019 17:22
@gcatron gcatron force-pushed the set_symbol_input_output_runtime_bundle branch 2 times, most recently from f5c6851 to daba9f5 Compare April 15, 2019 17:43
Copy link
Contributor

@jfix71 jfix71 left a comment

Choose a reason for hiding this comment

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

LGTM, just a few small nits.

Also wondering if it would be a good idea to add a QuantizationProfileNode to the test, so that you also check that a Placeholder that's not an output via a SaveNode is still marked as output, and that it's also marked as both input and output.

@gcatron gcatron force-pushed the set_symbol_input_output_runtime_bundle branch 3 times, most recently from 0ae19de to 6ed2150 Compare April 15, 2019 22:00
@nadavrot
Copy link
Contributor

Nice!

@gcatron gcatron force-pushed the set_symbol_input_output_runtime_bundle branch from 6ed2150 to 6117878 Compare April 15, 2019 23:11
@gcatron gcatron merged commit 1260d74 into pytorch:master Apr 15, 2019
@gcatron gcatron deleted the set_symbol_input_output_runtime_bundle branch April 15, 2019 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[runtime] Pass Input Output Placeholder labels
7 participants