-
-
Notifications
You must be signed in to change notification settings - Fork 11.9k
[BugFix] Fix non detected failing tests #30277
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
[BugFix] Fix non detected failing tests #30277
Conversation
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.
Code Review
This pull request addresses failing tests by making necessary adjustments to test configurations. In tests/compile/test_compile_ranges.py, the SchedulerConfig instantiations are updated to include the now-required max_model_len and is_encoder_decoder parameters, which resolves test failures related to this configuration change. In tests/compile/test_pass_manager.py, the test test_pass_manager_uuid is correctly wrapped with a pass_context to fix an issue where PassManager.uuid() was being called without the necessary context. The changes are accurate and effectively fix the described issues. The code is clean and well-targeted.
zou3519
left a comment
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.
thank you
|
I guess I will read the test logs after CI runs |
|
CI needs this fix to correctly detect the errors. Running this locally |
643b55a to
d47bf93
Compare
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
d47bf93 to
2c68110
Compare
| self.is_last_graph = piecewise_compile_index == total_piecewise_compiles - 1 | ||
|
|
||
| self.is_full_graph = total_piecewise_compiles == 1 | ||
| # TODO: we need to generalize encoder compilation to other models |
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.
Could we move this to config init ewhere we init the range in the first place?
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 init compilation config once. But we need to use different ranges for encoder and decoder compilations. We should mark the type of compilation somehow in a follow-up PR.
Purpose
Compilation tests were using the find command, which does not propagate errors, so the CI passes even when tests fail.
Fix compilation tests find commands.
Fixes tests that were not previously detected by CI:
compile/test_pass_manager.py::test_pass_manager_uuidFailures were caused by usage of PassManager.uuid without pass_context.
Test failures were caused by enforced non-default fields in SchedulerConfig.
compile/fullgraph/test_multiple_graphs.pyFixes circular import
compile/fullgraph/test_multimodal_compile.py::test_qwen2_5_vl_compilationIncreases upper bound of the last compile range in case of encoder blocks compilation.
Probably, needs a general way to detect encoder compilation in PiecewiseBackend.
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.