[https://nvbugs/5823783][test] add qa test case for trust-remote-code on multinode failure - #11905
Conversation
705f4c6 to
50379e1
Compare
|
/bot run |
📝 WalkthroughWalkthroughThe PR introduces a new unit test for validating pickle serialization of tokenizers with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unittest/llmapi/test_tokenizer_multinode.py (1)
10-10: Use module import + namespace access for tokenizer helpers.Line 10 imports class/function symbols directly from a module, which conflicts with the repo’s Python import policy. Please import the module and access members via namespace on Line 51 and Line 60.
♻️ Proposed fix
-from tensorrt_llm.tokenizer.tokenizer import TransformersTokenizer, load_hf_tokenizer +from tensorrt_llm.tokenizer import tokenizer @@ - tok = load_hf_tokenizer("Kimi-K2-Instruct", trust_remote_code=True) + tok = tokenizer.load_hf_tokenizer("Kimi-K2-Instruct", trust_remote_code=True) @@ - assert isinstance(restored, TransformersTokenizer) + assert isinstance(restored, tokenizer.TransformersTokenizer)As per coding guidelines: "Python imports must use form
from package.subpackage import module(neverfrom module import Class)."Also applies to: 51-51, 60-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unittest/llmapi/test_tokenizer_multinode.py` at line 10, The test imports symbols directly—TransformersTokenizer and load_hf_tokenizer—from tensorrt_llm.tokenizer.tokenizer which violates the import policy; change the import to the module form (import the tokenizer module, e.g., from tensorrt_llm.tokenizer import tokenizer) and update usages at the call sites that reference TransformersTokenizer and load_hf_tokenizer (the occurrences around the previously noted lines) to use namespace access (tokenizer.TransformersTokenizer and tokenizer.load_hf_tokenizer).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unittest/llmapi/test_tokenizer_multinode.py`:
- Around line 44-56: The test mutates sys.modules entries
("transformers_modules" and "transformers_modules.kimi_k2.tokenization_kimi")
without preserving prior values, risking cross-test contamination; modify the
test around load_hf_tokenizer/KimiK2Tokenizer to save the original values (e.g.,
orig_tm = sys.modules.get("transformers_modules") and orig_sub =
sys.modules.get("transformers_modules.kimi_k2.tokenization_kimi")), set the fake
modules, run the mocked AutoTokenizer.from_pretrained and pickle roundtrip as
before, then in the finally block restore the originals (put back
orig_tm/orig_sub if not None, otherwise pop the keys) so existing shared state
is preserved.
---
Nitpick comments:
In `@tests/unittest/llmapi/test_tokenizer_multinode.py`:
- Line 10: The test imports symbols directly—TransformersTokenizer and
load_hf_tokenizer—from tensorrt_llm.tokenizer.tokenizer which violates the
import policy; change the import to the module form (import the tokenizer
module, e.g., from tensorrt_llm.tokenizer import tokenizer) and update usages at
the call sites that reference TransformersTokenizer and load_hf_tokenizer (the
occurrences around the previously noted lines) to use namespace access
(tokenizer.TransformersTokenizer and tokenizer.load_hf_tokenizer).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: deb500d0-8d35-4c3e-8cb1-f0fb5c99fd14
📒 Files selected for processing (2)
tests/integration/test_lists/qa/llm_function_core.txttests/unittest/llmapi/test_tokenizer_multinode.py
|
PR_Github #37670 [ run ] triggered by Bot. Commit: |
|
PR_Github #37670 [ run ] completed with state
|
|
/bot run |
|
PR_Github #37784 [ run ] triggered by Bot. Commit: |
JunyiXu-nv
left a comment
There was a problem hiding this comment.
The test logic looks good, but the bug should be tested on multinode senario. Is this one running on multi-node?
This case even does not requires gpu, as I use a clean subprocess to simulate the multinode env. And has checked this case will fail on v1.2.0rc8, with error message: And on main branch, which includes your fix PR, can pass. So I think this is enough for check this bug, without running on a real multinode env. |
|
PR_Github #37784 [ run ] completed with state
|
|
/bot run |
|
PR_Github #37969 [ run ] triggered by Bot. Commit: |
|
PR_Github #37969 [ run ] completed with state |
Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
afcc26f to
29f69a3
Compare
|
/bot reuse-pipeline |
|
PR_Github #38181 [ reuse-pipeline ] triggered by Bot. Commit: |
|
PR_Github #38181 [ reuse-pipeline ] completed with state |
… on multinode failure (NVIDIA#11905) Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
… on multinode failure (NVIDIA#11905) Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
… on multinode failure (NVIDIA#11905) Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
has checked this case can capture the multinode failure:
on bad commit: [FAIL] https://prod.blsm.nvidia.com/swqa-tensorrt-qa-test/job/DEBUG_LLM_FUNCTION_TEST/2327/
on good commit: [PASS] https://prod.blsm.nvidia.com/swqa-tensorrt-qa-test/job/DEBUG_LLM_FUNCTION_TEST/2328
Summary by CodeRabbit
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.