[https://nvbugs/6221621][test] Update trust_remote to nemotron and phi4 models - #14570
Conversation
…ean up waives.txt Removed outdated model paths and unnecessary entries from MODEL_PATH_DICT in test_perf.py. Updated waives.txt to reflect the removal of tests that are no longer applicable, improving clarity and maintainability. Signed-off-by: yufeiwu-nv <230315618+yufeiwu-nv@users.noreply.github.com>
Signed-off-by: yufeiwu-nv <230315618+yufeiwu-nv@users.noreply.github.com>
These 7 waivers referenced perf tests (bart_large_cnn, bert_large, flan_t5_base/large/xl/xxl, mbart_large_50_many_to_one_mmt) that no longer appear in any test-db yaml on main. Drop them to keep the cleanup consistent with the 5 sibling waivers (roberta_base, t5_*) that were already removed in this PR. Signed-off-by: yufeiwu-nv <230315618+yufeiwu-nv@users.noreply.github.com>
Drop the 4 perf waivers that the PR originally added — author confirmed the underlying nvbugs (5150255 / 5304388 / 6130334) are no longer necessary to waive. Signed-off-by: yufeiwu-nv <230315618+yufeiwu-nv@users.noreply.github.com>
Included additional models "nemotron_nano_12b_v2", "phi_4_multimodal_instruct", "phi_4_multimodal_instruct_fp4", and "phi_4_multimodal_instruct_fp8" to the TRUST_REMOTE_CODE_MODELS dictionary to enhance testing coverage. Signed-off-by: yufeiwu-nv <230315618+yufeiwu-nv@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR refactors model-path configuration for performance integration tests by extracting shared lookup tables ( ChangesPerf test model path refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/defs/perf/_model_paths.py (1)
17-147: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd
__all__to declare the module’s public interface.This module is now a shared constants API, but it doesn’t declare exported names via
__all__.Proposed patch
"""Shared model path constants for perf and perf-sanity tests.""" +__all__ = ["MODEL_PATH_DICT", "HF_MODEL_PATH", "LORA_MODEL_PATH"] + # Model PATH of local dir synced from internal LLM models repo MODEL_PATH_DICT = {As per coding guidelines: "Despite the prohibition on wildcard imports, keep
__all__updated to document the public interface".🤖 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 `@tests/integration/defs/perf/_model_paths.py` around lines 17 - 147, Add a module-level __all__ listing the public constants to document and control the exported API; include the three symbols MODEL_PATH_DICT, HF_MODEL_PATH, and LORA_MODEL_PATH (i.e. set __all__ = ["MODEL_PATH_DICT", "HF_MODEL_PATH", "LORA_MODEL_PATH"]) so imports/export behavior is explicit for callers of this shared constants module.
🤖 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.
Outside diff comments:
In `@tests/integration/defs/perf/_model_paths.py`:
- Around line 17-147: Add a module-level __all__ listing the public constants to
document and control the exported API; include the three symbols
MODEL_PATH_DICT, HF_MODEL_PATH, and LORA_MODEL_PATH (i.e. set __all__ =
["MODEL_PATH_DICT", "HF_MODEL_PATH", "LORA_MODEL_PATH"]) so imports/export
behavior is explicit for callers of this shared constants module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 048d27f8-5e7b-4a4d-b2bd-d3da0f79c321
📒 Files selected for processing (4)
tests/integration/defs/perf/_model_paths.pytests/integration/defs/perf/test_perf.pytests/integration/defs/perf/test_perf_sanity.pytests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
|
/bot skip --comment "test modify" |
|
PR_Github #50387 [ skip ] triggered by Bot. Commit: |
|
PR_Github #50387 [ skip ] completed with state |
|
/bot skip --comment "test modify" |
|
PR_Github #50466 [ skip ] triggered by Bot. Commit: |
|
PR_Github #50466 [ skip ] completed with state |
…i4 models (NVIDIA#14570) Signed-off-by: yufeiwu-nv <230315618+yufeiwu-nv@users.noreply.github.com>
Summary by CodeRabbit
Description
Included additional models "nemotron_nano_12b_v2", "phi_4_multimodal_instruct", "phi_4_multimodal_instruct_fp4", and "phi_4_multimodal_instruct_fp8" to the TRUST_REMOTE_CODE_MODELS dictionary to enhance testing coverage.
Signed-off-by: yufeiwu-nv 230315618+yufeiwu-nv@users.noreply.github.com
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)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.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.