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

Avoid using uncessary get_values(MODEL_MAPPING) #29362

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Avoid using uncessary get_values(MODEL_MAPPING) #29362

merged 2 commits into from
Feb 29, 2024

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Feb 29, 2024

What does this PR do?

When we switch to torch 2.2 in daily CI, natten causing issue and I temporarily disabled its installation. However, some test files use model_class in get_values(MODEL_MAPPING) which will fail with

RuntimeError: Failed to import transformers.models.dinat.modeling_dinat because of the following error (look up to see its traceback):

even if those models are not requiring natten.

The usage of

get_values(MODEL_MAPPING)`

is slow and unnecessary, we can just use the names (string) rather than import them to get the classes.

This PR changes them to use the class name strings.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@ydshieh
Copy link
Collaborator Author

ydshieh commented Feb 29, 2024

cc @NielsRogge For future vision models, let's avoid using get_values() 🙏

@ydshieh ydshieh removed the request for review from ArthurZucker February 29, 2024 04:12
@ydshieh
Copy link
Collaborator Author

ydshieh commented Feb 29, 2024

Hmm, sorry there is still a tiny issue. Working on it

Comment on lines +738 to +741
"PerceiverForImageClassificationLearned",
"PerceiverForImageClassificationFourier",
"PerceiverForImageClassificationConvProcessing",
*MODEL_FOR_IMAGE_CLASSIFICATION_MAPPING_NAMES.values(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using stuff like MODEL_FOR_IMAGE_CLASSIFICATION_MAPPING_NAMES.values() is not reliable, as an element in MODEL_FOR_IMAGE_CLASSIFICATION_MAPPING_NAMES.values() maybe a list itself. For example, we have

        (
            "perceiver",
            (
                "PerceiverForImageClassificationLearned",
                "PerceiverForImageClassificationFourier",
                "PerceiverForImageClassificationConvProcessing",
            ),
        ),

in MODEL_FOR_IMAGE_CLASSIFICATION_MAPPING_NAMES.

It's probably better to have something like _get_names_for_values: similar to get_values in src/transformers/models/auto/auto_factory.py but only return names without loading the classes.

I could do that in a follow up PR, or any better suggestion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah it's faster also to not load the classes and just compare strings

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Love that it is simpler 😉

Comment on lines +738 to +741
"PerceiverForImageClassificationLearned",
"PerceiverForImageClassificationFourier",
"PerceiverForImageClassificationConvProcessing",
*MODEL_FOR_IMAGE_CLASSIFICATION_MAPPING_NAMES.values(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah it's faster also to not load the classes and just compare strings

@ydshieh ydshieh merged commit 44fe1a1 into main Feb 29, 2024
18 checks passed
@ydshieh ydshieh deleted the fix_dinat branch February 29, 2024 09:19
@ydshieh ydshieh mentioned this pull request Mar 14, 2024
itazap pushed a commit that referenced this pull request May 14, 2024
* more fixes

* more fixes

---------

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
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.

3 participants