Skip to content

Exporters onnx module#403

Merged
michaelbenayoun merged 39 commits intohuggingface:mainfrom
michaelbenayoun:exporters_onnx_module
Nov 2, 2022
Merged

Exporters onnx module#403
michaelbenayoun merged 39 commits intohuggingface:mainfrom
michaelbenayoun:exporters_onnx_module

Conversation

@michaelbenayoun
Copy link
Member

@michaelbenayoun michaelbenayoun commented Sep 27, 2022

What does this PR do?

This PR adds the ONNX export to optimum, under optimum.exporters.onnx, which was previously supported in transformers. This also comes with a refactoring.

  • DummyInputGenerator classes have been added: their goal is to handle the generation of dummy inputs. This can be used in any sitution, and is not tied to the ONNX export at all.
  • All the base classes that were present in transformers are kept, but they now use DummyInputGenerators to generate dummy inputs. This means that there is no need to provide a tokenizer or a feature extractor to the OnnxConfig.
  • "Intermediate" configuration classes have been added to represent a middle-end between the base generic class, and the model specific onnx config class: EncoderOnnxConfig, DecoderOnnxConfig, Seq2SeqOnnxConfig, VisionOnnxConfig, TextAndVisionOnnxConfig, etc.
  • Massive class inheritance for model specific onnx config classes when possible

What's left?

  • DummyInputGenerators docstring
  • Automatic feature detection using the Hub (@julien-c) (Exporters automatic task detection #445)
  • Fixing some axis naming issue observed for some models
  • Making all the models pass the export test
  • Should we keep the attention_mask as input when not needed? Or make it an option?
  • Add support for audio tasks
  • Validate models with a different shape than the one used to trace
  • Allow 2 "versions" of tests:
    • One which is fast, and lightweight for the CI to not take too long and fail from memory issues
    • One which is slower, and performs the export on much bigger models

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 27, 2022

The documentation is not available anymore as the PR was closed or merged.

@flozi00
Copy link

flozi00 commented Sep 29, 2022

Any plans or advice about adding speech related tasks ?

@michaelbenayoun
Copy link
Member Author

Hi @flozi00 ,
Yes the plan is to cover as much models as possible, so we will definitely support speech related tasks. It will happen in multiple PRs though.

Copy link
Contributor

@regisss regisss left a comment

Choose a reason for hiding this comment

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

Thanks for this huuuge work @michaelbenayoun! I have a few comments and I also spotted a few typos.

Copy link
Contributor

@regisss regisss left a comment

Choose a reason for hiding this comment

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

Huge work @michaelbenayoun 🚀 🚀 🚀

Copy link
Member

@lewtun lewtun left a comment

Choose a reason for hiding this comment

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

Amazing work @michaelbenayoun - I really like the elegant approach to using inheritance with the ONNX configs!

I've left a few comments, but overall this looks really nice. The main question is whether we should use this feature as an opportunity to clean up some name choices we made in transformers

Also, I think this PR should copy-paste the serialization.mdx docs from transformers to optimum (with the appropriate changes of course)

# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
"""Model export features manager."""
Copy link
Member

Choose a reason for hiding this comment

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

I always found the word "features" a bit confusing in the original exporter - does it refer to feature vectors or something else?

Since we have the opportunity to do something new, would something like "architecture" or "head" be more appropriate?

Copy link
Member

Choose a reason for hiding this comment

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

👍 for architecture, or what about just pipeline type or task type?

More generally 100% agree that we could use this opportunity to improve the DX as much as we can

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I like:

  • TaskManager
  • ArchitectureManager

Copy link
Contributor

Choose a reason for hiding this comment

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

I fully agree that "features" sounds odd here.

My 2 cents: "task" sounds great, especially since people with little ML knowledge will understand it instantly whereas "head" will not mean much to them, "architecture" is as confusing as "features" IMO and "pipeline" can mean many different things.

Choose a reason for hiding this comment

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

either task or pipeline would be consistent with the rest of the Hub. If consistent with the Hub, then task fits better. If consistent with transformers, then pipeline.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went for task!
Actually I think that pipeline is not suitted because we do not match the pipeline names. Like sequence-classification instead of text-classification.

Copy link
Member

Choose a reason for hiding this comment

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

Task sounds good (even if we can't 100% match the Hub definitions because of past key-value variants + the fact we're really mapping "task" to AutoModelForXxx).

)

ExportConfigConstructor = Callable[[PretrainedConfig], "ExportConfig"]
FeatureNameToExportConfigDict = Dict[str, ExportConfigConstructor]
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to include the type in this object? Wouldn't FeatureNameToExportConfig be better?

if self.use_past:
common_inputs["attention_mask"][1] = "past_encoder_sequence + sequence"
common_inputs["decoder_input_ids"] = {0: "batch"}
# common_inputs["decoder_attention_mask"] = {0: "batch", 1: "past_decoder_sequence + sequence"}
Copy link
Member

Choose a reason for hiding this comment

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

Delete?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, still not sure about this.

}


class AlbertOnnxConfig(BertOnnxConfig):
Copy link
Member

Choose a reason for hiding this comment

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

Very elegant!

@lewtun lewtun mentioned this pull request Oct 26, 2022
3 tasks
Copy link
Contributor

@regisss regisss left a comment

Choose a reason for hiding this comment

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

Huuuge PR @michaelbenayoun, thanks 🚀

# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
"""Model export features manager."""
Copy link
Contributor

Choose a reason for hiding this comment

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

I fully agree that "features" sounds odd here.

My 2 cents: "task" sounds great, especially since people with little ML knowledge will understand it instantly whereas "head" will not mean much to them, "architecture" is as confusing as "features" IMO and "pipeline" can mean many different things.

@michaelbenayoun michaelbenayoun merged commit 5972cc7 into huggingface:main Nov 2, 2022
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.

7 participants