Skip to content

[None][fix] Fix DFlash budget test broken by CachedModelLoader signature change - #19582

Merged
weiminwang-nv merged 1 commit into
NVIDIA:mainfrom
weiminwang-nv:fix/dflash-test-cached-model-loader-signature
Sep 23, 2026
Merged

weiminwang-nv merged 1 commit into
NVIDIA:mainfrom
weiminwang-nv:fix/dflash-test-cached-model-loader-signature

Conversation

@weiminwang-nv

@weiminwang-nv weiminwang-nv commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Description

TestHubIdRevalidation in tests/unittest/_torch/speculative/hw_agnostic/test_dflash_config_budget.py fails with TypeError on current main.

The test was added by #19466, which builds the loader as:

CachedModelLoader(args, llm_build_stats=None, workspace=str(tmp_path))

#19428 merged ~9 hours later and removed both llm_build_stats and workspace from CachedModelLoader.__init__ (along with the LlmBuildStats class and the workspace property). The two PRs touch tensorrt_llm/llmapi/llm_utils.py in nearby but non-overlapping hunks, so there was no textual merge conflict and neither PR's CI run saw the other's change.

This patch drops the removed keyword arguments. workspace was only passed to avoid creating a TemporaryDirectory, which CachedModelLoader no longer does, so the tmp_path argument of the _loader helper is dropped as well. No behavior under test changes.

Affected tests:

  • TestHubIdRevalidation::test_hub_id_budget_checked_after_download
  • TestHubIdRevalidation::test_local_path_is_not_checked_twice

Test Coverage

tests/unittest/_torch/speculative/hw_agnostic/test_dflash_config_budget.py::TestHubIdRevalidation

PR Checklist

  • PR title is following PR title format
  • Test cases are provided for new code paths added
  • Any new dependencies have been scanned for license and vulnerabilities
  • Documentation updated as needed

Dev Engineer Review

_loader no longer accepts the unused tmp_path argument or passes the removed llm_build_stats and workspace arguments to CachedModelLoader. The change is limited to the test helper and its callers. The stated behavior under test is unchanged.

QA Engineer Review

The modified unit test file covers hub-ID budget validation after download and avoids checking a local path twice. The helper change aligns its constructor call with the current CachedModelLoader API. No test-list changes are reported. Coverage verdict: sufficient for the API-compatibility change.

Per-File QA Perspective

  • tests/unittest/_torch/speculative/hw_agnostic/test_dflash_config_budget.py: The tests cover budget checks after download and for local paths. This is a unit test file; integration CI and manual-QA listing status is not established by the supplied information.

@weiminwang-nv

Copy link
Copy Markdown
Collaborator Author

/bot run

@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: NVIDIA/TensorRT-LLM/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a8933c62-8675-4531-a6a1-a9df0bbf3f1d

📥 Commits

Reviewing files that changed from the base of the PR and between 950c923 and 7bcdd25.

📒 Files selected for processing (1)
  • tests/unittest/_torch/speculative/hw_agnostic/test_dflash_config_budget.py

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


Walkthrough

The DFlash configuration budget tests simplify _loader and update its Hub-ID and local-path callers.

Changes

DFlash budget tests

Layer / File(s) Summary
Update loader helper and callers
tests/unittest/_torch/speculative/hw_agnostic/test_dflash_config_budget.py
_loader no longer accepts tmp_path or passes llm_build_stats and workspace to CachedModelLoader. Both callers now pass only speculative_model.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix

Suggested reviewers: brnguyen2

Merge Risk: ⚪ Minimal · up to 7bcdd

The loader update preserves both budget tests’ assertions and uses the supported constructor signature; no material merge risk is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title follows the required [ticket][type] format and clearly describes the fix for the broken DFlash budget test caused by the CachedModelLoader signature change.
Description check ✅ Passed The description explains the failure, identifies the removed CachedModelLoader arguments, describes the solution, lists affected tests, and provides test coverage. Some checklist items are not explici…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #75214 [ run ] triggered by Bot. Commit: 7bcdd25 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #75214 [ run ] completed with state FAILURE. Commit: 7bcdd25
/LLM/main/L0_MergeRequest_PR pipeline #61963 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@github-actions

Copy link
Copy Markdown

Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements.

@weiminwang-nv

Copy link
Copy Markdown
Collaborator Author

/bot run --stage-list="CPU-Generic-x86-1,CPU-Generic-arm-1"

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #75227 [ run ] triggered by Bot. Commit: 7bcdd25 Link to invocation

…ure change

PR NVIDIA#19466 added TestHubIdRevalidation, which constructs CachedModelLoader
with llm_build_stats and workspace. PR NVIDIA#19428 landed afterwards and removed
both parameters (together with LlmBuildStats and the workspace property), so
the two tests in that class now fail with TypeError on main.

Drop the removed keyword arguments. The workspace argument only existed to
avoid creating a TemporaryDirectory, which CachedModelLoader no longer does,
so the tmp_path argument of the _loader helper is no longer needed either.

Signed-off-by: Weimin Wang <301118019+weiminwang-nv@users.noreply.github.com>
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #75227 [ run ] completed with state SUCCESS. Commit: 7bcdd25
/LLM/main/L0_MergeRequest_PR pipeline #61977 (Partly Tested) completed with status: 'SUCCESS'

CI Report

Link to invocation

@weiminwang-nv
weiminwang-nv force-pushed the fix/dflash-test-cached-model-loader-signature branch 2 times, most recently from 7bcdd25 to a05b335 Compare September 23, 2026 10:36
@weiminwang-nv

Copy link
Copy Markdown
Collaborator Author

/bot skip --comment "test cases change only. All affected test stages are passed"

@weiminwang-nv
weiminwang-nv enabled auto-merge (squash) September 23, 2026 10:38
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #75242 [ skip ] triggered by Bot. Commit: a05b335 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #75242 [ skip ] completed with state SUCCESS. Commit: a05b335
Skipping testing for commit a05b335

Link to invocation

@weiminwang-nv
weiminwang-nv merged commit d79ecd5 into NVIDIA:main Sep 23, 2026
11 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants