fix: fix some trtllm tests #3084
Conversation
|
/ok to test c35169d |
WalkthroughUpdates test expectations for aggregated router logs and chat response in TRT-LLM serve tests, and adds a brief delay before validating expected logs in the engine process to allow KV events to appear. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test
participant EP as EngineProcess.check_response
participant L as Logs
participant V as validate_expected_logs
T->>EP: check_response(payload with expected_log)
EP->>EP: If expected_log set
Note over EP: Delay to allow KV event logs
EP-->>EP: sleep(0.5)
EP->>V: validate_expected_logs(expected_log, L)
V-->>EP: validation result
EP-->>T: return (assertions proceed)
rect rgb(230,245,255)
note right of T: Separate change
T->>T: chat_payload_default(expected_response=["Hello world!"])
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. 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: 0
🧹 Nitpick comments (3)
tests/utils/engine_process.py (2)
112-118: Replace fixed sleep with bounded polling to de-flake log assertions.A hardcoded
time.sleep(0.5)is brittle (can be too short on loaded CI and unnecessarily slow when logs are already present). Recommend polling the logs until patterns appear or a bounded timeout elapses, with an env‑configurable deadline.Apply this diff to the call site:
- if payload.expected_log: - time.sleep( - 0.5 - ) # The kv event sometimes needs extra time to arrive and be reflected in the log. - self.validate_expected_logs(payload.expected_log) + if payload.expected_log: + self.validate_expected_logs( + payload.expected_log, + max_wait_secs=float(os.getenv("DYNAMO_LOG_WAIT_SECS", "3.0")), + poll_interval=0.1, + )Then update
validate_expected_logsto support polling (see snippet below).def validate_expected_logs(self, patterns: Any, max_wait_secs: float = 0.0, poll_interval: float = 0.1) -> None: """Validate that all regex patterns are present in the current logs, optionally waiting up to max_wait_secs.""" import re import time deadline = time.time() + max(0.0, max_wait_secs) compiled = [re.compile(p) for p in patterns] while True: content = self.read_logs() or "" if content: missing = [p for p, rx in zip(patterns, compiled) if not rx.search(content)] if not missing: logger.info(f"SUCCESS: All expected log patterns: {patterns} found") return # Exit if no waiting requested or deadline reached if max_wait_secs <= 0.0 or time.time() >= deadline: sample = content[-1000:] if content and len(content) > 1000 else content if not content: raise EngineLogError(f"Log file not available or empty at path: {self.log_path}") raise EngineLogError(f"Missing expected log patterns: {missing}\n\nLog sample:\n{sample}") time.sleep(poll_interval)
7-7: Drop top‑leveltimeimport if polling uses local import.To stay consistent with the existing “local import to keep module load minimal” style, remove the module‑level
import timeonce the polling logic imports it locally.-import timetests/serve/test_trtllm.py (1)
111-113: Align expectation with logits processor output; avoid brittle punctuation.Docstring says “Hello world”, while the assertion requires “Hello world!”. To reduce brittleness, accept both.
- chat_payload_default(expected_response=["Hello world!"]), + chat_payload_default(expected_response=["Hello world", "Hello world!"]),Also applies to: 125-126
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/serve/test_trtllm.py(1 hunks)tests/utils/engine_process.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/serve/test_trtllm.py (1)
tests/utils/payload_builder.py (1)
chat_payload_default(13-36)
⏰ 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: Build and Test - dynamo
🔇 Additional comments (1)
tests/serve/test_trtllm.py (1)
60-66: LGTM: ZMQ listener pattern removed; remaining patterns match TRT‑LLM path.The expected log patterns now reflect the router/scheduler flow without assuming ZMQ listener logs.
|
Thanks for fixing this. Was my mistakes when I did the refactor. @nv-tusharma was also fixing these separately in his trtllm container refactor PR. #3009 |
Thanks @alec-flowers @nv-tusharma ! I need to merge this since the CI code merge gate is blocked on this at the moment. |
Signed-off-by: Kristen Kelleher <[email protected]>
Overview:
https://gitlab-master.nvidia.com/dl/ai-dynamo/dynamo/-/jobs/210682524
Fix some failing trtllm tests
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit