Skip to content

Conversation

@ilmarkov
Copy link
Contributor

@ilmarkov ilmarkov commented Dec 8, 2025

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:

  1. compile/test_pass_manager.py::test_pass_manager_uuid
    Failures were caused by usage of PassManager.uuid without pass_context.

compile/test_compile_ranges.py::test_compile_ranges
compile/test_compile_ranges.py::test_compile_config_get_compile_ranges
compile/test_compile_ranges.py::test_inductor_cache_compile_ranges

Test failures were caused by enforced non-default fields in SchedulerConfig.

  1. compile/fullgraph/test_multiple_graphs.py
    Fixes circular import

  2. compile/fullgraph/test_multimodal_compile.py::test_qwen2_5_vl_compilation
    Increases 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
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Copy link
Collaborator

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

thank you

@zou3519 zou3519 added the ready ONLY add when PR is ready to merge/full CI is needed label Dec 8, 2025
@zou3519
Copy link
Collaborator

zou3519 commented Dec 8, 2025

I guess I will read the test logs after CI runs

@ilmarkov
Copy link
Contributor Author

ilmarkov commented Dec 8, 2025

CI needs this fix to correctly detect the errors.

Running this locally find compile/ -maxdepth 1 -name 'test_*.py' -print0 | xargs -0 -n1 -I{} pytest -s -v '{}' so far.

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>
@ilmarkov ilmarkov force-pushed the imarkov/fix_compilation_tests branch from d47bf93 to 2c68110 Compare December 9, 2025 15:25
@zou3519 zou3519 enabled auto-merge (squash) December 9, 2025 15:53
@zou3519 zou3519 merged commit 0b6a8a3 into vllm-project:main Dec 9, 2025
48 checks passed
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
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/build ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants