Skip to content
This repository was archived by the owner on Jun 3, 2025. It is now read-only.

Conversation

@natuan
Copy link
Contributor

@natuan natuan commented Jul 14, 2021

This allows for passing in inputs of dict type to the export_onnx, enabling exported model with the original inp/out names.

@natuan natuan requested review from a team, bfineran, kevinaer, markurtz and mgoin and removed request for a team July 14, 2021 14:24
Copy link
Contributor

@bfineran bfineran left a comment

Choose a reason for hiding this comment

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

LGTM overall, I'm worried that for models with multiple inputs, the proper input order will not be preserved by a Dict unless it is an OrderedDict.

It might be safer to have input_names as a separate arg whose names should match the order of the sample input. Having a warning if the Dict is not ordered might also be ok though. If we keep Dict the transformers integration should be updated to use an ordered one.

@natuan natuan force-pushed the onnx_export_with_dict branch from 7eaa477 to 85bf86c Compare July 15, 2021 15:52
@natuan
Copy link
Contributor Author

natuan commented Jul 15, 2021

LGTM overall, I'm worried that for models with multiple inputs, the proper input order will not be preserved by a Dict unless it is an OrderedDict.

It might be safer to have input_names as a separate arg whose names should match the order of the sample input. Having a warning if the Dict is not ordered might also be ok though. If we keep Dict the transformers integration should be updated to use an ordered one.

Added the followings:

  • Warning if sample batch passed in is a dict but not an ordered dict
  • From transformers side, the sample batch constructed as OrderedDict

bfineran
bfineran previously approved these changes Jul 15, 2021
Copy link
Contributor

@bfineran bfineran left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @natuan!

@bfineran
Copy link
Contributor

looks like quality needs to run @natuan

@natuan natuan force-pushed the onnx_export_with_dict branch from 42c1ab8 to e4f1172 Compare July 15, 2021 17:17
@markurtz markurtz merged commit 1f45a58 into main Jul 16, 2021
@markurtz markurtz deleted the onnx_export_with_dict branch September 1, 2021 11:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants