Skip to content

[skip ci][Docs] reorganize multiple L4 test guidelines#2119

Merged
Gaohan123 merged 8 commits into
vllm-project:mainfrom
fhfuih:diffuion-l4-test-doc
Mar 30, 2026
Merged

[skip ci][Docs] reorganize multiple L4 test guidelines#2119
Gaohan123 merged 8 commits into
vllm-project:mainfrom
fhfuih:diffuion-l4-test-doc

Conversation

@fhfuih
Copy link
Copy Markdown
Contributor

@fhfuih fhfuih commented Mar 24, 2026

Signed-off-by: Huang, Zeyu 11222265+fhfuih@users.noreply.github.com

Purpose

Currently, the detailed guidelines for L4 tests are very long. Because there are multiple test scopes, each evolving into well-developed, templated design.

Plus, per previous PR comment, the current place for L4 diffusion functionality test guideline is not very good. After that PR is merged with coding templates, this PR can focus on the relevant doc improvement.

This PR

Test Plan

NA

Test Result

NA


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. Please provide the test scripts & test commands. Please state the reasons if your codes don't require additional test scripts. For test file guidelines, please check the test style doc
  • The test results. Please paste the results comparison before and after, or the e2e results.
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model. Please run mkdocs serve to sync the documentation editions to ./docs.
  • (Optional) Release notes update. If your change is user-facing, please update the release notes draft.

BEFORE SUBMITTING, PLEASE READ https://github.com/vllm-project/vllm-omni/blob/main/CONTRIBUTING.md (anything written below this line will be removed by GitHub Actions)

Signed-off-by: Huang, Zeyu <11222265+fhfuih@users.noreply.github.com>
@fhfuih fhfuih force-pushed the diffuion-l4-test-doc branch from 9403ed4 to 7d59aa7 Compare March 24, 2026 06:27
@fhfuih fhfuih marked this pull request as ready for review March 24, 2026 06:28
Copilot AI review requested due to automatic review settings March 24, 2026 06:28
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Reorganizes and modularizes the L4 testing guidelines documentation so the L4 “Test Examples” section in CI_5levels.md is split into focused include fragments, and updates the diffusion-model onboarding doc to point to the consolidated L1–L5 testing guide.

Changes:

  • Replaced the long, diffusion-specific L4 test guideline block in adding_diffusion_model.md with a pointer to the multi-level CI/testing doc.
  • Added three new L4 include fragments for documentation-example tests, performance tests, and functionality tests.
  • Refactored CI_5levels.md to include the new fragments via snippet includes.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
docs/contributing/model/adding_diffusion_model.md Points diffusion model contributors to the centralized CI/test-level docs instead of inline L4 guidance.
docs/contributing/ci/test_examples/l4_performance_tests.inc.md New reusable fragment describing how to add L4 performance tests via tests/dfx/perf/tests/test.json.
docs/contributing/ci/test_examples/l4_functionality_tests.inc.md New reusable fragment describing L4 diffusion functionality test scope/design/style.
docs/contributing/ci/test_examples/l4_doc_example_tests.inc.md New reusable fragment describing strategies/naming/runtime rules for doc example tests.
docs/contributing/ci/CI_5levels.md Replaces the inlined L4 “Test Examples” content with snippet includes to the new fragments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread docs/contributing/ci/test_examples/l4_functionality_tests.inc.md Outdated
Comment thread docs/contributing/ci/test_examples/l4_performance_tests.inc.md Outdated
Comment thread docs/contributing/model/adding_diffusion_model.md Outdated
Comment thread docs/contributing/ci/test_examples/l4_functionality_tests.inc.md Outdated
Comment thread docs/contributing/ci/test_examples/l4_performance_tests.inc.md Outdated
1. Change the ---xxx-xx-xx running parameters to xxx_xx_xx format and fill them as keys in the JSON file.
2. For boolean variables in the running parameters, modify them to forms such as ignore_eos: true/false and fill them into the JSON file.
3. Add the baseline parameter to specify the required validation values, ensuring the validation metric names match those in the result.json generated by the benchmark.
4. The qps and concurrency modes are mutually exclusive. For detailed explanations, see the table below:
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

This says QPS and concurrency modes are mutually exclusive, but the perf runner supports specifying both request_rate and max_concurrency lists and will run a QPS sweep and a separate concurrency sweep. Consider rephrasing to clarify they are run as separate sweeps rather than disallowed together.

Suggested change
4. The qps and concurrency modes are mutually exclusive. For detailed explanations, see the table below:
4. QPS-based (`request_rate`) and concurrency-based (`max_concurrency`) benchmarks are configured independently. If you specify both, the perf runner will run a QPS sweep and a separate concurrency sweep. For detailed explanations, see the table below:

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I confirmed that they can be used together, but when combined, they may affect each other, causing the request rate to not necessarily reach the set value. If modification is needed, this can be described accordingly, or simply recommend that users use them separately.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Resolve by simply "recommend" users to use them exclusively

Comment thread docs/contributing/ci/test_examples/l4_performance_tests.inc.md Outdated
fhfuih and others added 4 commits March 24, 2026 14:33
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Zeyu Huang | 黃澤宇 <11222265+fhfuih@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Zeyu Huang | 黃澤宇 <11222265+fhfuih@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Zeyu Huang | 黃澤宇 <11222265+fhfuih@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Zeyu Huang | 黃澤宇 <11222265+fhfuih@users.noreply.github.com>
@fhfuih
Copy link
Copy Markdown
Contributor Author

fhfuih commented Mar 24, 2026

@wtomin @congw729 @yenuo26 PTAL. And @congw729 See if the remaining AI comments on the performance test is applicable, I can accept or decline them. (Or if you have the permission to "sign off and commit suggestion" on your end, you can also do it)

@congw729
Copy link
Copy Markdown
Collaborator

@wtomin @congw729 @yenuo26 PTAL. And @congw729 See if the remaining AI comments on the performance test is applicable, I can accept or decline them. (Or if you have the permission to "sign off and commit suggestion" on your end, you can also do it)

Please also run mkdocs serve to check the layout is good.

@fhfuih
Copy link
Copy Markdown
Contributor Author

fhfuih commented Mar 24, 2026

Please also run mkdocs serve to check the layout is good.

Sure. It works fine on my end:

image
image
image
image

fhfuih added 3 commits March 30, 2026 14:32
Signed-off-by: Huang, Zeyu <11222265+fhfuih@users.noreply.github.com>
Signed-off-by: Huang, Zeyu <11222265+fhfuih@users.noreply.github.com>
Signed-off-by: Huang, Zeyu <11222265+fhfuih@users.noreply.github.com>
@Gaohan123 Gaohan123 added the ready label to trigger buildkite CI label Mar 30, 2026
Copy link
Copy Markdown
Collaborator

@Gaohan123 Gaohan123 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@Gaohan123 Gaohan123 merged commit 0cf9914 into vllm-project:main Mar 30, 2026
3 checks passed
@fhfuih fhfuih deleted the diffuion-l4-test-doc branch March 31, 2026 01:15
vraiti pushed a commit to vraiti/vllm-omni that referenced this pull request Apr 9, 2026
…2119)

Signed-off-by: Huang, Zeyu <11222265+fhfuih@users.noreply.github.com>
Signed-off-by: Zeyu Huang | 黃澤宇 <11222265+fhfuih@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
lengrongfu pushed a commit to lengrongfu/vllm-omni that referenced this pull request May 1, 2026
…2119)

Signed-off-by: Huang, Zeyu <11222265+fhfuih@users.noreply.github.com>
Signed-off-by: Zeyu Huang | 黃澤宇 <11222265+fhfuih@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
clodaghwalsh17 pushed a commit to clodaghwalsh17/nm-vllm-omni-ent that referenced this pull request May 12, 2026
…2119)

Signed-off-by: Huang, Zeyu <11222265+fhfuih@users.noreply.github.com>
Signed-off-by: Zeyu Huang | 黃澤宇 <11222265+fhfuih@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready label to trigger buildkite CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants