Skip to content

[Training/Python] Add option to enable symbolic shape inference#5107

Merged
ke1337 merged 9 commits intomasterfrom
kedeng/sym
Sep 22, 2020
Merged

[Training/Python] Add option to enable symbolic shape inference#5107
ke1337 merged 9 commits intomasterfrom
kedeng/sym

Conversation

@ke1337
Copy link
Copy Markdown
Contributor

@ke1337 ke1337 commented Sep 10, 2020

and allow save model to directory

Description: Describe your changes.

Motivation and Context

  • Why is this change required? What problem does it solve?
  • If it fixes an open issue, please link to the issue here.

# returns True if no more ready nodes
def _insert_ready_nodes():
ready_nodes = [pn for pn in pending_nodes if all([i in defined for i in pn.input if i])]
ready_nodes = [pn for pn in pending_nodes if len(pn.input) == 0 or all([i in defined for i in pn.input if i])]
Copy link
Copy Markdown
Contributor

@RandySheriffH RandySheriffH Sep 11, 2020

Choose a reason for hiding this comment

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

len(pn.input) == 0 [](start = 57, length = 19)

nit:

if not pn.input #Resolved

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, rewritten this part


In reply to: 487200955 [](ancestors = 487200955)

Copy link
Copy Markdown
Contributor

@thiagocrepaldi thiagocrepaldi left a comment

Choose a reason for hiding this comment

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


self._verify_fully_optimized_model(self.onnx_model_)

if self.save_model_dir or self.run_symbolic_shape_infer:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What scenarios are being unlocked by this change on ORTTrainer? Should this be done for both training and evaluation? InferenceSession is handled by https://github.com/microsoft/onnxruntime/blob/master/onnxruntime/python/session.py#L158

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

BART needs this change, so most tensors could be statically planned. Inference does not rely on shape info for memory pattern though. (https://github.com/microsoft/onnxruntime/blob/master/onnxruntime/core/framework/session_state.cc#L313-L352)


In reply to: 488801960 [](ancestors = 488801960)

Copy link
Copy Markdown
Contributor

@liqunfu liqunfu left a comment

Choose a reason for hiding this comment

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

Eventually we want merge shape inference function into ort c++ side.

@SherlockNoMad
Copy link
Copy Markdown
Contributor

Looks good to me, great to have this as an option!

@ke1337 ke1337 merged commit 8dceebd into master Sep 22, 2020
@ke1337 ke1337 deleted the kedeng/sym branch September 22, 2020 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants