Skip to content

[None][test] Don't force disable_overlap_scheduler for perf-sanity ctx_only tests - #18346

Merged
chenfeiz0326 merged 1 commit into
NVIDIA:mainfrom
chenfeiz0326:feat/ctx-only-honor-overlap-scheduler
Aug 28, 2026
Merged

[None][test] Don't force disable_overlap_scheduler for perf-sanity ctx_only tests#18346
chenfeiz0326 merged 1 commit into
NVIDIA:mainfrom
chenfeiz0326:feat/ctx-only-honor-overlap-scheduler

Conversation

@chenfeiz0326

@chenfeiz0326 chenfeiz0326 commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Description

The ctx_only mode of the perf-sanity suite (aggr[_upload]-ctx_only-<disagg config>)
builds an aggregated server config out of the disaggregated yaml's worker_config.ctx
section. While doing so it unconditionally overwrote the ctx worker's
disable_overlap_scheduler with True:

# Disable overlap scheduler for ctx_only
ctx_config["disable_overlap_scheduler"] = True

The overlap scheduler works fine for a context-only run, so this override is removed and
the value comes from the yaml like every other worker_config.ctx knob.

90 of the 96 CI disagg configs set ctx disable_overlap_scheduler: false (since #17390),
so their e2e / gen_only tests already run the ctx worker with the overlap scheduler
enabled while the ctx_only test generated from the same config file ran it disabled. The
6 gb300_nemotron-ultra-v3-fp4_* configs that set ctx disable_overlap_scheduler: true
keep running with the overlap scheduler off.

When the key is absent from a yaml, ServerConfig already defaults it to False, which
matches both the LLM API default and the aggregated tests. Spec-decode modes that cannot
support the overlap scheduler are still handled by the runtime: py_executor_creator.py
sets disable_overlap_scheduler = True itself and logs a warning when
spec_config.spec_dec_mode.support_overlap_scheduler() is false.

Impact on baselines

This changes the measured configuration of the existing ctx_only cases (90 of 96 flip from
overlap-off to overlap-on), so their perf baselines are expected to shift once after merge.

Test Coverage

Harness-only change. Verified by driving _parse_disagg_config_file in ctx_only mode
over all 96 configs in tests/scripts/perf-sanity/disaggregated/ and inspecting the
generated extra-llm-api-config content:

ctx value in yaml emitted disable_overlap_scheduler configs
false false 90
true true 6
key absent key omitted (engine default) synthetic case

cache_transceiver_config is still dropped in every case.

PR Checklist

  • PR description clearly explains what and why.

  • PR follows the TRT-LLM coding guidelines.

  • Harness-only change; verified against all 96 CI disagg configs (see Test Coverage). No API change.

  • No new dependencies.

  • No documentation change needed.

  • Please check this after reviewing the above items as appropriate for this PR.

Note on pre-commit: ruff format --check, ruff check and the whitespace/EOL checks were
run against the modified file and pass. The full pre-commit run --all-files could not be
executed in this environment (hook virtualenv creation fails on a missing platformdirs),
so please flag anything the CI formatting stage catches.

GitHub Bot Help

To see a list of available CI bot commands, please comment /bot help.

Dev Engineer Review

  • ctx_only now honors worker_config.ctx.disable_overlap_scheduler.
  • The change removes the unconditional True override.
  • Missing YAML values use the ServerConfig default of False.
  • Explicit true values remain unchanged.
  • The scope is limited to the intended performance-sanity test behavior.
  • No API, error-handling, or test-list changes were identified.
  • Review verdict: sufficient.

QA Engineer Review

  • Modified test function: ctx_only.
  • The test function is covered by the existing CI disaggregated configurations.
  • Validation covered all 96 configurations.
  • Ninety configurations now use the default or YAML-defined value.
  • Six configurations explicitly retain disable_overlap_scheduler: true.
  • Spec-decode runtime handling remains unchanged.
  • Review verdict: sufficient.

@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 361b6105-152a-4055-b38a-de2cfbcc798c

📥 Commits

Reviewing files that changed from the base of the PR and between c96a463 and 0df65b2.

📒 Files selected for processing (1)
  • tests/integration/defs/perf/test_perf_sanity.py
💤 Files with no reviewable changes (1)
  • tests/integration/defs/perf/test_perf_sanity.py

Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.


Walkthrough

The performance test no longer forces disable_overlap_scheduler to True when ctx_only is enabled. ServerConfig now uses the YAML value, or False when the setting is absent.

Changes

ctx_only configuration handling

Layer / File(s) Summary
Preserve YAML scheduler setting
tests/integration/defs/perf/test_perf_sanity.py
The ctx_only path passes the YAML-defined disable_overlap_scheduler value to ServerConfig. YAML omission retains the False default.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to 0df65

This localized harness change lets ctx-only performance tests honor the scheduler setting from their configuration; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: bowenfu

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the test change: perf-sanity ctx_only tests no longer force disable_overlap_scheduler. It uses the expected ticket and type format.
Description check ✅ Passed The description includes the required Description, Test Coverage, and PR Checklist sections. It explains the issue, the solution, validation across all 96 configurations, baseline impact, and checklis…
Full details: Description check

Explanation

The description includes the required Description, Test Coverage, and PR Checklist sections. It explains the issue, the solution, validation across all 96 configurations, baseline impact, and checklist status.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

…x_only tests

The ctx_only mode builds an aggregated server config out of the disaggregated
yaml's worker_config.ctx section, and unconditionally overwrote
disable_overlap_scheduler with True. The overlap scheduler works fine for a
context-only run, so use the value from the yaml like every other
worker_config.ctx knob.

All but 6 of the CI disagg configs set ctx disable_overlap_scheduler: false, so
their e2e/gen_only tests already run the ctx worker with the overlap scheduler
enabled while ctx_only ran it disabled. The perf baselines of those ctx_only
cases are expected to shift once.

Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com>
@chenfeiz0326
chenfeiz0326 force-pushed the feat/ctx-only-honor-overlap-scheduler branch from c96a463 to 0df65b2 Compare August 28, 2026 05:26
@chenfeiz0326 chenfeiz0326 changed the title [None][test] Honor ctx yaml disable_overlap_scheduler in perf-sanity ctx_only tests [None][test] Don't force disable_overlap_scheduler for perf-sanity ctx_only tests Aug 28, 2026

@fredricz-20070104 fredricz-20070104 left a comment

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.

Approved.

@chenfeiz0326
chenfeiz0326 enabled auto-merge (squash) August 28, 2026 05:45
@chenfeiz0326

Copy link
Copy Markdown
Collaborator Author

/bot skip --comment "Only update config for ctx only perf tests, no need to run the whole CI pipeline"

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #69895 [ skip ] triggered by Bot. Commit: 0df65b2 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #69895 [ skip ] completed with state SUCCESS. Commit: 0df65b2
Skipping testing for commit 0df65b2

Link to invocation

@chenfeiz0326
chenfeiz0326 merged commit 263921f into NVIDIA:main Aug 28, 2026
14 checks passed
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.

5 participants