Skip to content
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

ORTModule support for kwargs input that is a dict #13910

Merged
merged 9 commits into from
Dec 15, 2022

Conversation

baijumeswani
Copy link
Contributor

ORTModule has support *args, **kwargs since it was added earlier.
ORTModule also has support for nested lists and dictionaries.

However, it does not support model inputs that are a part of **kwargs as well as are dicts. This was probably not thought of as support needed earlier. A model that @frank-dong-ms has been working on needs this support.

With this pull-request, ORTModule can successfully run such models.

contributors: @frank-dong-ms, @baijumeswani

@baijumeswani baijumeswani added the training issues related to ONNX Runtime training; typically submitted using template label Dec 9, 2022
Copy link
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.

Nice addition

@shubhambhokare1 might be interested in seeing a real use case for nested dicts

Copy link
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.

LGTM

@@ -113,21 +135,15 @@ def __repr__(self) -> str:
\tRequire gradient: {self.require_grad_names}
\tDynamic axes: {self.dynamic_axes}
\tSchema: {self.schema}
\t#Positionals (total): {self.num_positionals}
\t#Expanded Positionals (non-None): {self.num_expanded_positionals_non_none}
\tKeyword names: {self.keyword_names}"""
Copy link
Contributor

Choose a reason for hiding this comment

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

By not printing the captured names, we make debugging of scenarios with kwargs harder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Let me merge this change to unblock a model, and I will create a follow up PR to add the flattened keyword names as part of the InputInfo.

Copy link
Contributor

@askhade askhade left a comment

Choose a reason for hiding this comment

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

LGTM

@baijumeswani baijumeswani merged commit 1fd6348 into main Dec 15, 2022
@baijumeswani baijumeswani deleted the bmeswani/ortmodule-kwargs-dict branch December 15, 2022 00:23
henrywu2019 pushed a commit to henrywu2019/onnxruntime that referenced this pull request Dec 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
training issues related to ONNX Runtime training; typically submitted using template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants