[https://nvbugs/6337224][fix] Update PERF_SANITY_DIR to include aggregated/; in recipe_to_server_config… - #15484
Conversation
…h committed YAMLs Two issues caused tests/unittest/tools/test_config_database_sync.py:: test_config_database_tests_sync to fail: 1. PERF_SANITY_DIR pointed at tests/scripts/perf-sanity/ but the committed config_database_*.yaml files were moved to perf-sanity/aggregated/ in PR NVIDIA#11802 (commit 22c4706). The sync test's glob found no files, while the regenerator emitted two, producing a filename-set mismatch. 2. PR NVIDIA#14438 (commit 26c099f) hand-edited the committed b200/h200 YAMLs to add server_env_var: "TLLM_SPEC_DECODE_FORCE_NUM_ACCEPTED_TOKENS=<N>" on every entry whose recipe had speculative_config.max_draft_len, but the regenerator was never updated. After fix NVIDIA#1, the test surfaced this content drift on per-file YAML comparison. Fix the regenerator to: - write/glob from tests/scripts/perf-sanity/aggregated/ - emit server_env_var = "TLLM_SPEC_DECODE_FORCE_NUM_ACCEPTED_TOKENS=<max_draft_len>" whenever the recipe's loaded LLM API config has speculative_config.max_draft_len, with the value emitted as a double-quoted YAML scalar to match the committed style (consistent with sibling YAMLs in the same directory). Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
📝 WalkthroughWalkthroughThe script adds a ChangesSpec-decode env var injection in generated test configs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/generate_config_database_tests.py (1)
99-114: ⚡ Quick winMake
server_env_varprecedence explicit to avoid silent overwrite.
server_env_varis injected first, but then thellm_api_configcopy loop can overwrite it. Moving forced env injection after the copy loop makes the deterministic override behavior explicit.♻️ Proposed refactor
- spec_env = ( - { - "server_env_var": _DoubleQuoted( - f"TLLM_SPEC_DECODE_FORCE_NUM_ACCEPTED_TOKENS={max_draft_len}" - ) - } - if max_draft_len is not None - else {} - ) - server_config = { "name": generate_server_name(recipe), - **spec_env, "model_name": model_name, "gpus": recipe.num_gpus, # Enable scenario-only matching for baseline comparison "match_mode": "scenario", } # Copy LLM API config fields for key, value in llm_api_config.items(): server_config[key] = value + + if max_draft_len is not None: + server_config["server_env_var"] = _DoubleQuoted( + f"TLLM_SPEC_DECODE_FORCE_NUM_ACCEPTED_TOKENS={max_draft_len}" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/generate_config_database_tests.py` around lines 99 - 114, The server_env_var from spec_env is currently being unpacked into server_config early (using **spec_env), but this can be silently overwritten by subsequent operations that copy llm_api_config into server_config. Move the unpacking of spec_env to after the llm_api_config is copied into server_config so that the forced speculative decoding environment variable takes explicit precedence and is not accidentally overwritten. This ensures the deterministic max_draft_len override is guaranteed to be preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/generate_config_database_tests.py`:
- Line 56: The deeper nested path in PERF_SANITY_DIR can cause FileNotFoundError
when writing generated YAML files if the directory doesn't exist beforehand. Add
an explicit mkdir call for test_config_dir near the beginning of the
generate_tests function to ensure the full directory structure is created before
any file write operations occur, particularly when using custom output
directories.
---
Nitpick comments:
In `@scripts/generate_config_database_tests.py`:
- Around line 99-114: The server_env_var from spec_env is currently being
unpacked into server_config early (using **spec_env), but this can be silently
overwritten by subsequent operations that copy llm_api_config into
server_config. Move the unpacking of spec_env to after the llm_api_config is
copied into server_config so that the forced speculative decoding environment
variable takes explicit precedence and is not accidentally overwritten. This
ensures the deterministic max_draft_len override is guaranteed to be preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0552d63c-4dc6-4596-a355-274054536913
📒 Files selected for processing (1)
scripts/generate_config_database_tests.py
|
/bot run --disable-fail-fast --stage-list "A10-PyTorch-1" |
|
PR_Github #58611 [ run ] triggered by Bot. Commit: |
|
PR_Github #58611 [ run ] completed with state |
|
/bot run --disable-fail-fast --stage-list "A10-PyTorch-1" |
|
/bot kill |
|
/bot skip --comment "Fix a ut, no need to run the whole CI pipeline" |
|
PR_Github #62517 [ run ] triggered by Bot. Commit: |
|
PR_Github #62519 [ kill ] triggered by Bot. Commit: |
|
PR_Github #62517 [ run ] completed with state |
|
PR_Github #62519 [ kill ] completed with state |
|
PR_Github #62520 [ skip ] triggered by Bot. Commit: |
|
PR_Github #62520 [ skip ] completed with state |
4 participants
Summary
aggregated/subdir where PR [https://nvbugs/5846166][fix] Update Perf Triage Scripts to Fix gen_only issue #11802 moved the files.aggregated/; inrecipe_to_server_config, whenspeculative_config.max_draft_lenis present in the loaded recipe config, setserver_env_varto the corresponding TLLM_SPEC_DECODE_FORCE_NUM_ACCEPTED_TOKENS string, wrapped in a_DoubleQuotedstr subclass with a custom yaml representer to preserve the committed double-quoted style.Test plan
Links
Summary by CodeRabbit