[https://nvbugs/5453727][fix] Fix bug of how GPT-OSS setup the parameters in CI #7151
Conversation
📝 WalkthroughWalkthroughStandardized GSM8K evaluation in GPT-OSS PyTorch integration tests by introducing a class-level extra_evaluator_kwargs, removing per-test task mutation, patching GSM8K.MAX_OUTPUT_LEN to 8192 in relevant tests, and updating test signatures to accept mocker/monkeypatch for the patches. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test runner
participant C as TestGPTOSS (class)
participant G as GSM8K Task
participant L as LLM
note over C: extra_evaluator_kwargs defined (multi-turn, few-shot, chat-template, output limits)
T->>C: setup test (inject mocker/monkeypatch)
T->>G: patch MAX_OUTPUT_LEN = 8192
T->>G: G.evaluate(L, extra_evaluator_kwargs=C.extra_evaluator_kwargs)
G->>L: generate(answer, respecting chat-template & limits)
L-->>G: output
G-->>T: evaluation result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.py📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
Files:
**/*.{c,cc,cpp,cxx,h,hh,hpp,hxx,cu,cuh,py}📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
Files:
🧬 Code graph analysis (1)tests/integration/defs/accuracy/test_llm_api_pytorch.py (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
byshiue
changed the title
|
/bot run --stage-list "B200_PCIe-PyTorch-1, B200_PCIe-PyTorch-2, B200_PCIe-PyTorch-3" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tensorrt_llm/evaluate/lm_eval.py (1)
401-404: Gate GSM8K debug output behind logger/env var; avoid print+flush and local import.Direct
import sysstrictly for flushing is unnecessary. Preferloggerand optionally guard with an env var to keep CI output sane. Also, current message prints raw scores before normalization; call this out in the log to avoid confusion.Apply this diff:
- if self.task_name == "gsm8k": - print(f"scores: {scores}, results: {results}") - import sys - sys.stdout.flush() + if self.task_name == "gsm8k" and os.environ.get("TRTLLM_DEBUG_LMEVAL"): + logger.info("gsm8k raw scores (pre-normalization): %s", scores) + logger.debug("lm-eval full results: %s", results)Optional: If you actually want normalized values in the debug log, move the logging block to after the normalization loop at Line 408.
tests/integration/defs/accuracy/test_llm_api_pytorch.py (1)
2311-2313: Don’t comment out evaluations in tests; feature-gate MMLU instead.Commenting code reduces traceability and is easy to forget. Gate it with an env var so you can toggle in CI without code edits, while keeping GSM8K intact.
Apply this diff:
- # task = MMLU(self.MODEL_NAME) - # task.evaluate(llm) + if os.getenv("TRTLLM_SKIP_MMLU_NVFP4", "0") != "1": + task = MMLU(self.MODEL_NAME) + task.evaluate(llm) + else: + print("Skipping MMLU for NVFP4 (TRTLLM_SKIP_MMLU_NVFP4=1)")If there’s a known issue, consider linking it in a comment or switching to a targeted skip mark (e.g., pytest.mark.skipif at the test method) once a stable condition is available.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tensorrt_llm/evaluate/lm_eval.py(1 hunks)tests/integration/defs/accuracy/test_llm_api_pytorch.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
**/*.py: Python code must target Python 3.8+
Python indentation: 4 spaces, no tabs
Maintain module namespace in imports (from package.subpackage import foo; then use foo.SomeClass())
Python file names use snake_case
Python class names use PascalCase
Python functions/methods and local variables use snake_case; variables starting with a number get k_ prefix (e.g., k_99th_percentile)
Global variables use G_ prefixed UPPER_SNAKE_CASE (e.g., G_MY_GLOBAL)
Constants use UPPER_SNAKE_CASE in Python
Avoid shadowing variables from outer scopes in Python
Initialize all externally visible members of a Python class in init
Prefer docstrings for interfaces used outside a file; comments for local code
Use Google-style docstrings for classes and functions (Sphinx-parsable)
Document attributes/variables inline with short docstrings
Avoid reflection when simple alternatives exist (e.g., prefer explicit parameters over dict(**locals()))
In try/except, catch the narrowest exceptions possible
For duck-typing with try/except, keep try body minimal and put logic in else
Files:
tensorrt_llm/evaluate/lm_eval.pytests/integration/defs/accuracy/test_llm_api_pytorch.py
**/*.{cpp,cxx,cc,cu,h,hpp,hxx,hh,cuh,py}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
Prepend NVIDIA copyright header (current year) to all source files
Files:
tensorrt_llm/evaluate/lm_eval.pytests/integration/defs/accuracy/test_llm_api_pytorch.py
🧠 Learnings (1)
📚 Learning: 2025-07-28T17:06:08.621Z
Learnt from: moraxu
PR: NVIDIA/TensorRT-LLM#6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.
Applied to files:
tests/integration/defs/accuracy/test_llm_api_pytorch.py
|
PR_Github #16151 [ run ] triggered by Bot |
|
/bot run --stage-list "B200_PCIe-PyTorch-2" |
|
PR_Github #16188 [ run ] triggered by Bot |
|
PR_Github #16151 [ run ] completed with state |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/integration/defs/accuracy/accuracy_core.py(1 hunks)tests/integration/defs/accuracy/test_llm_api_pytorch.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/defs/accuracy/test_llm_api_pytorch.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
**/*.py: Python code must target Python 3.8+
Python indentation: 4 spaces, no tabs
Maintain module namespace in imports (from package.subpackage import foo; then use foo.SomeClass())
Python file names use snake_case
Python class names use PascalCase
Python functions/methods and local variables use snake_case; variables starting with a number get k_ prefix (e.g., k_99th_percentile)
Global variables use G_ prefixed UPPER_SNAKE_CASE (e.g., G_MY_GLOBAL)
Constants use UPPER_SNAKE_CASE in Python
Avoid shadowing variables from outer scopes in Python
Initialize all externally visible members of a Python class in init
Prefer docstrings for interfaces used outside a file; comments for local code
Use Google-style docstrings for classes and functions (Sphinx-parsable)
Document attributes/variables inline with short docstrings
Avoid reflection when simple alternatives exist (e.g., prefer explicit parameters over dict(**locals()))
In try/except, catch the narrowest exceptions possible
For duck-typing with try/except, keep try body minimal and put logic in else
Files:
tests/integration/defs/accuracy/accuracy_core.py
**/*.{cpp,cxx,cc,cu,h,hpp,hxx,hh,cuh,py}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
Prepend NVIDIA copyright header (current year) to all source files
Files:
tests/integration/defs/accuracy/accuracy_core.py
|
/bot run --stage-list "B200_PCIe-PyTorch-2" |
|
PR_Github #16236 [ run ] triggered by Bot |
|
PR_Github #16236 [ run ] completed with state |
|
/bot run |
|
PR_Github #16261 [ run ] triggered by Bot |
|
PR_Github #16261 [ run ] completed with state |
|
/bot run --stage-list "B200_PCIe-PyTorch-2" |
|
PR_Github #16265 [ run ] triggered by Bot |
|
PR_Github #16265 [ run ] completed with state |
Signed-off-by: bhsueh <[email protected]>
Signed-off-by: bhsueh <[email protected]>
Signed-off-by: bhsueh <[email protected]>
Signed-off-by: bhsueh <[email protected]>
|
/bot run --stage-list "B200_PCIe-PyTorch-2" |
|
PR_Github #16274 [ run ] triggered by Bot |
|
PR_Github #16274 [ run ] completed with state |
Signed-off-by: bhsueh <[email protected]>
|
/bot run --stage-list "B200_PCIe-PyTorch-2" |
|
PR_Github #16277 [ run ] triggered by Bot |
|
PR_Github #16277 [ run ] completed with state |
|
PR_Github #16491 [ run ] completed with state |
Signed-off-by: bhsueh <[email protected]>
|
/bot run --stage-list "B200_PCIe-PyTorch-2" |
|
PR_Github #16513 [ run ] triggered by Bot |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/integration/defs/accuracy/accuracy_core.py (1)
302-302: Revert GSM8K NUM_SAMPLES=2; this collapses the hypothesis test and effectively disables accuracy gatingHardcoding NUM_SAMPLES to 2 inflates the threshold magnitude (|z_alpha|sqrt(2sigma^2/n)), making failures unlikely and hiding regressions. The trailing comment “Full sample” is also incorrect for value 2.
Apply this minimal fix:
- NUM_SAMPLES = 2 # Full sample + NUM_SAMPLES = 1319 # Full sampleOptionally, keep debug agility without harming defaults by adding an env override (see my separate comment on get_num_samples_and_threshold).
To illustrate impact locally:
#!/bin/bash python - <<'PY' import math sigma=50.0 z_alpha=-1.6448536269514729 for n in (1319,2): scale=(2*sigma*sigma/n)**0.5 shift=z_alpha*scale print(f"n={n:4d} scale={scale:6.2f} threshold_shift={shift:7.2f}") PY
🧹 Nitpick comments (2)
tests/integration/defs/accuracy/accuracy_core.py (2)
191-199: Replace raw prints with logger and gate behind an env; also support env override for apply_chat_template at construction timeRaw prints add noise to CI logs and cannot be filtered. Use logger and make the diagnostics opt-in. While here, add an env-controlled apply_chat_template override so debugging doesn’t require code edits.
Suggested patch:
- evaluator_kwargs = {} - print(f"self.EVALUATOR_KWARGS: {self.EVALUATOR_KWARGS}") - print(f"extra_evaluator_kwargs: {extra_evaluator_kwargs}") + evaluator_kwargs = {} + if os.getenv("TRTLLM_DEBUG_EVAL_KWARGS") == "1": + logger.info(f"EVALUATOR_KWARGS(base): {self.EVALUATOR_KWARGS}") + logger.info(f"EVALUATOR_KWARGS(extra): {extra_evaluator_kwargs}") if self.EVALUATOR_KWARGS is not None: evaluator_kwargs.update(self.EVALUATOR_KWARGS) if extra_evaluator_kwargs is not None: evaluator_kwargs.update(extra_evaluator_kwargs) + # Optional apply_chat_template override (per-dataset or global) + _ds = getattr(self, "DATASET", "").upper() + _override = os.getenv(f"TRTLLM_APPLY_CHAT_TEMPLATE_OVERRIDE_{_ds}", + os.getenv("TRTLLM_APPLY_CHAT_TEMPLATE_OVERRIDE")) + if _override is not None: + val = str(_override).strip().lower() in {"1","true","yes","on"} + prev = evaluator_kwargs.get("apply_chat_template", None) + if prev != val: + logger.info(f"Overriding apply_chat_template for {self.DATASET}: {prev} -> {val} via env") + evaluator_kwargs["apply_chat_template"] = val + if os.getenv("TRTLLM_DEBUG_EVAL_KWARGS") == "1": + logger.info(f"EVALUATOR_KWARGS(merged): {evaluator_kwargs}") evaluator = self.EVALUATOR_CLS(num_samples=num_samples, **evaluator_kwargs)
122-134: Add optional environment-based override fornum_samplesTo keep the full‐sample defaults for hypothesis testing while enabling quick CI or local debug downsampling, insert an env-override immediately after the
num_samples = entry.get(...)line in
tests/integration/defs/accuracy/accuracy_core.py.• File: tests/integration/defs/accuracy/accuracy_core.py
• Insert after line wherenum_samplesis set (just beforehigher_is_better = ...).sigma = entry.get("sigma", self.SIGMA) num_samples = entry.get("num_samples", self.NUM_SAMPLES) + # Optional override for debug/CI: dataset-specific takes precedence over global. + override = os.getenv( + f"TRTLLM_NUM_SAMPLES_OVERRIDE_{self.DATASET.upper()}", + os.getenv("TRTLLM_NUM_SAMPLES_OVERRIDE"), + ) + if override: + try: + ov = max(1, int(override)) + if ov != num_samples: + logger.info( + f"Overriding num_samples for {self.DATASET} " + f"from {num_samples} to {ov} via env." + ) + num_samples = ov + except ValueError: + logger.warning( + f"Ignoring invalid TRTLLM_NUM_SAMPLES_OVERRIDE* value: {override!r}" + ) higher_is_better = entry.get("higher_is_better", self.HIGHER_IS_BETTER)This preserves the existing defaults and lets you opt into lower sample counts—either globally via
TRTLLM_NUM_SAMPLES_OVERRIDEor per‐dataset viaTRTLLM_NUM_SAMPLES_OVERRIDE_<DATASET>.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/integration/defs/accuracy/accuracy_core.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
**/*.py: Code must target Python 3.8+
Indent Python code with 4 spaces; do not use tabs
Preserve module namespaces when importing; import modules/packages and access members via the module (e.g., from package.subpackage import foo; foo.SomeClass())
Python file names should be snake_case
Python class names should be PascalCase
Python functions/methods and local variables should be snake_case; variables beginning with a number should be prefixed with k_ (e.g., k_99th_percentile)
Global variables should be UPPER_SNAKE_CASE prefixed with G_ (e.g., G_MY_GLOBAL); constants should be UPPER_SNAKE_CASE
Avoid shadowing variables from outer scopes; initialize all externally visible members in init
Prefer docstrings for interfaces used outside a file; comments should be reserved for in-function or file-local interfaces
Use Google-style docstrings for classes and functions; attributes and variables may be documented inline with trailing string literals
Avoid reflection when simpler, explicit code suffices (e.g., avoid dict(**locals()) patterns)
In try/except, catch the narrowest exceptions possible
For duck-typing patterns, keep the try body minimal and move logic to else to avoid masking unrelated failures
Files:
tests/integration/defs/accuracy/accuracy_core.py
**/*.{c,cc,cpp,cxx,h,hh,hpp,hxx,cu,cuh,py}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
Prepend the NVIDIA copyright header (current year) to all source files (.cpp, .h, .cu, .py, etc.)
Files:
tests/integration/defs/accuracy/accuracy_core.py
🧬 Code graph analysis (1)
tests/integration/defs/accuracy/accuracy_core.py (3)
tensorrt_llm/evaluate/interface.py (1)
evaluate(81-110)tensorrt_llm/evaluate/lm_eval.py (4)
evaluate(390-430)GSM8K(453-501)apply_chat_template(64-75)apply_chat_template(192-242)tensorrt_llm/inputs/utils.py (1)
apply_chat_template(407-446)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (1)
tests/integration/defs/accuracy/accuracy_core.py (1)
308-311: Further verify tests for GSM8K to see if any rely on the hard-coded default
We need to confirm whether any existing GSM8K tests assume theapply_chat_template=Falsedefault (i.e. without an explicitextra_evaluator_kwargs).
Please run:rg -n 'GSM8K' -C3 tests/integration/defs/accuracy/test_llm_api.pyand also:
rg -n 'GSM8K' -C3 tests/integration/defs/accuracy/test_llm_api_pytorch.pyto ensure no tests implicitly depend on the new default.
byshiue
changed the title
|
PR_Github #16513 [ run ] completed with state |
Signed-off-by: bhsueh <[email protected]>
|
/bot run --stage-list "B200_PCIe-PyTorch-2" |
|
PR_Github #16535 [ run ] triggered by Bot |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/integration/defs/accuracy/test_llm_api_pytorch.py (1)
2350-2350: Remove the unconditional failure (or gate it behind an env flag).This hard-stops CI for every run and was already flagged earlier.
Apply one of the following:
- assert False + # Optional local tripwire for debugging + if os.getenv("TRTLLM_DEBUG_FORCE_FAIL") == "1": + pytest.fail("Debug: forced failure (TRTLLM_DEBUG_FORCE_FAIL=1)")Run to ensure no other unconditional failures remain in this folder:
#!/bin/bash rg -nP 'assert\s+False|pytest\.fail\(' tests/integration/defs/accuracy
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/integration/defs/accuracy/test_llm_api_pytorch.py(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
**/*.py: Code must target Python 3.8+
Indent Python code with 4 spaces; do not use tabs
Preserve module namespaces when importing; import modules/packages and access members via the module (e.g., from package.subpackage import foo; foo.SomeClass())
Python file names should be snake_case
Python class names should be PascalCase
Python functions/methods and local variables should be snake_case; variables beginning with a number should be prefixed with k_ (e.g., k_99th_percentile)
Global variables should be UPPER_SNAKE_CASE prefixed with G_ (e.g., G_MY_GLOBAL); constants should be UPPER_SNAKE_CASE
Avoid shadowing variables from outer scopes; initialize all externally visible members in init
Prefer docstrings for interfaces used outside a file; comments should be reserved for in-function or file-local interfaces
Use Google-style docstrings for classes and functions; attributes and variables may be documented inline with trailing string literals
Avoid reflection when simpler, explicit code suffices (e.g., avoid dict(**locals()) patterns)
In try/except, catch the narrowest exceptions possible
For duck-typing patterns, keep the try body minimal and move logic to else to avoid masking unrelated failures
Files:
tests/integration/defs/accuracy/test_llm_api_pytorch.py
**/*.{c,cc,cpp,cxx,h,hh,hpp,hxx,cu,cuh,py}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
Prepend the NVIDIA copyright header (current year) to all source files (.cpp, .h, .cu, .py, etc.)
Files:
tests/integration/defs/accuracy/test_llm_api_pytorch.py
🧬 Code graph analysis (1)
tests/integration/defs/accuracy/test_llm_api_pytorch.py (3)
tests/integration/defs/conftest.py (1)
llm_models_root(77-83)tensorrt_llm/evaluate/lm_eval.py (2)
GSM8K(453-501)evaluate(390-430)tests/integration/defs/accuracy/accuracy_core.py (3)
GSM8K(295-312)evaluate(147-208)evaluate(711-721)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (4)
tests/integration/defs/accuracy/test_llm_api_pytorch.py (4)
2697-2697: LGTM: adding mocker for targeted patching.The new mocker fixture is appropriate for the per-test constant override you’re doing below.
2730-2730: LGTM: mocker added to the 4-GPU test signature.
2763-2763: LGTM: adding monkeypatch/mocker for per-test overrides.Using monkeypatch for env and mocker for attribute patching is the right split.
2715-2718: Fixpatch.objectusage; no change needed forscores_filterhandlingThe only issue here is the incorrect use of
mocker.patch.objectwith a dict. The integration test harness already consumesscores_filterviaextra_evaluator_kwargs(and supports comma-separated metrics, as seen in other tests), so there’s no need to overrideEVALUATE_KWARGS.• Replace:
mocker.patch.object(GSM8K, {"MAX_OUTPUT_LEN": 8192})with:
mocker.patch.object(GSM8K, "MAX_OUTPUT_LEN", 8192)• Leave the call to
task.evaluate(..., extra_evaluator_kwargs=self.extra_evaluator_kwargs)unchanged—scores_filter="exact_match,flexible-extract"will be applied as intended.Likely an incorrect or invalid review comment.
|
PR_Github #16535 [ run ] completed with state |
|
/bot run --stage-list "B200_PCIe-PyTorch-2" |
|
PR_Github #16546 [ run ] triggered by Bot |
|
PR_Github #16546 [ run ] completed with state |
Signed-off-by: bhsueh <[email protected]>
byshiue
changed the title
|
/bot run |
|
PR_Github #16560 [ run ] triggered by Bot |
|
PR_Github #16560 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #16607 [ run ] triggered by Bot |
|
Related bugs: |
byshiue
changed the title
|
PR_Github #16607 [ run ] completed with state |
Summary by CodeRabbit