-
Notifications
You must be signed in to change notification settings - Fork 31.3k
Fix failing test in Glm4vMoeIntegrationTest #42488
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
Conversation
7990f11 to
7ae537f
Compare
|
Thank you @Sai-Suraj-27 I will take a closer look. |
|
It turns out an issue caused by The following script shows the issue, and if we removed the Have no extra time to dive into import unittest
from transformers.testing_utils import run_first
class Glm4vMoeModelTest(unittest.TestCase):
def test_foo(self):
assert 1 == 1
class Glm4vMoeIntegrationTest(unittest.TestCase):
model = None
@classmethod
def get_model(cls):
if cls.model is None:
cls.model = {"a": "b"}
return cls.model
@classmethod
def tearDownClass(cls):
if hasattr(cls, "model"):
del cls.model
def setUp(self):
pass
def tearDown(self):
pass
def test_foo2(self):
model = self.get_model()
assert 1 == 1
def test_foo3(self):
model = self.get_model()
assert 1 == 1
@run_first
def test_foo4(self):
model = self.get_model()
assert 1 == 1 |
| @classmethod | ||
| def get_model(cls): | ||
| if cls.model is None: | ||
| if not hasattr(cls, "model") or cls.model is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need not hasattr(cls, "model"). Any reason you add this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added it as an extra safety condition. Since python follows lazy evaluation for if-conditions if there is no attribute model, then it will not evaluate cls.model is None and directly assings the appropriate value. So, the Glm4vMoeIntegrationTest has no attribute model failure will never happen. But yes, I think we can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. CI Looks green.
7ae537f to
514f374
Compare
|
Thanks. We still have some issues, but that is irrelevant to this attribute and the run_first. So I am going to merge and talk to the team with the following issue |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: glm4v_moe |
What does this PR do?
Fixes these failing tests in Glm4vMoeIntegrationTest.
As discussed here, It depends on how pytest collects the different tests, so similar to Lfm2MoeIntegrationTest, Qwen3MoeIntegrationTest (which are not failing in their get_model() function calls with this issue) it would be ideal to have
setUpClassthat initializescls.modelalways.Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
@ydshieh @Cyrilvallez