Skip to content

[Pipelines] Extend pipelines to handle multiple possible AutoModel classes#11968

Closed
patrickvonplaten wants to merge 9 commits intohuggingface:masterfrom
patrickvonplaten:extend_pipelines_for_automodel_tupels
Closed

[Pipelines] Extend pipelines to handle multiple possible AutoModel classes#11968
patrickvonplaten wants to merge 9 commits intohuggingface:masterfrom
patrickvonplaten:extend_pipelines_for_automodel_tupels

Conversation

@patrickvonplaten
Copy link
Copy Markdown
Contributor

@patrickvonplaten patrickvonplaten commented May 31, 2021

What does this PR do?

This PR extends pipeline to better handle multiple auto model classes per pipeline.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@patrickvonplaten patrickvonplaten requested a review from Narsil May 31, 2021 23:10
@patrickvonplaten patrickvonplaten changed the title [Pipelines] Extend pipelines to handle multiple possible AutoModel classes [WIP][Pipelines] Extend pipelines to handle multiple possible AutoModel classes May 31, 2021
@patrickvonplaten patrickvonplaten changed the title [WIP][Pipelines] Extend pipelines to handle multiple possible AutoModel classes [Pipelines] Extend pipelines to handle multiple possible AutoModel classes May 31, 2021
Copy link
Copy Markdown
Contributor

@Narsil Narsil left a comment

Choose a reason for hiding this comment

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

I welcome this change

However, I have a feeling we're adding another extra layer of complexity.

Couldn't we use this PR, to simplify the overall logic here. Maybe None could become an empty tuple.
Single class could become 1-tuple.

Overall the rest of the flow should be more streamlined, don't you think ?

Also, IIRC, config.architectures was not added until some point in transformers. Do we have any way to check that we're not breaking legacy models ? (Scanning the hub is my best guess)

@patrickvonplaten
Copy link
Copy Markdown
Contributor Author

Re:

Also, IIRC, config.architectures was not added until some point in transformers. Do we have any way to check that we're not breaking legacy models ? (Scanning the hub is my best guess)

Agree that we should be careful here. I'm scanning the hub now to check, but I'm pretty sure that 99% of models have config.architectures. Even the very old models like gpt2 have config.architectures = [...] saved. Also, instead of raising an error if config.architectures doesn't exist, we could just pick the first element of the tuple -> this way we ensure that we cannot break anything that worked previously. What do you think?

Re:

However, I have a feeling we're adding another extra layer of complexity.

Couldn't we use this PR, to simplify the overall logic here. Maybe None could become an empty tuple.
Single class could become 1-tuple.

Overall the rest of the flow should be more streamlined, don't you think ?

Agree that we are adding more complexity, but I don't really see how to allow multiple auto classes without adding more complexity. I don't really see how forcing everything to be in the tuple format will help here. But keen to hear your proposition on how this could reduce overall complexity!

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.

2 participants