Skip to content

Conversation

@ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Feb 17, 2025

What does this PR do?

This might be just a temporary fix though.

@ydshieh ydshieh requested a review from zucchini-nlp February 17, 2025 10:02
@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.

Copy link
Member

@zucchini-nlp zucchini-nlp left a comment

Choose a reason for hiding this comment

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

Does this run a dummy check_config_can_be_init_without_params instead of fetching the newly added one?

I think we need the new test, to prevent regression. Maybe we can move to to test_modeling_llava.py and as you did, add to the ConfigTester?

@ydshieh
Copy link
Collaborator Author

ydshieh commented Feb 17, 2025

Hi ✋

In #36077, there is no newly added check_config_can_be_init_without_params. There is a newly added test file/class tests/models/llava/test_configuration_llava.py and relevant methods, but they are extra tests.

This PR only affects the original test_config in LlavaForConditionalGenerationModelTest in tests/models/llava/test_modeling_llava.py (which are independent from the new tests/models/llava/test_configuration_llava.py).

And it's not dummy: it simply select the else branch below (defined in tests/test_configuration_common.py)

    def check_config_can_be_init_without_params(self):
        if self.config_class.is_composition:
            with self.parent.assertRaises(ValueError):
                config = self.config_class()
        else:
            config = self.config_class()
            self.parent.assertIsNotNone(config)

@ydshieh ydshieh requested a review from zucchini-nlp February 17, 2025 10:38
Copy link
Member

@zucchini-nlp zucchini-nlp left a comment

Choose a reason for hiding this comment

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

Ahah I am dumb, sorry. Yeah, makes sense now, thanks for fixing!

@ydshieh ydshieh merged commit 23d6095 into main Feb 17, 2025
17 checks passed
@ydshieh ydshieh deleted the fix_check_config_can_be_init_without_params branch February 17, 2025 10:49
zucchini-nlp pushed a commit to zucchini-nlp/transformers that referenced this pull request Feb 21, 2025
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.

4 participants