Skip to content

fix(profiler): resolve local PVC model path for AIC calls - #9877

Merged
hhzhang16 merged 9 commits into
ai-dynamo:mainfrom
jooe0824:jooe0824/fix-profiler-resolve-model-path
May 26, 2026
Merged

fix(profiler): resolve local PVC model path for AIC calls#9877
hhzhang16 merged 9 commits into
ai-dynamo:mainfrom
jooe0824:jooe0824/fix-profiler-resolve-model-path

Conversation

@jooe0824

@jooe0824 jooe0824 commented May 22, 2026

Copy link
Copy Markdown
Contributor

rapid.py and thorough.py passed the raw HF model id to AIC even when modelCache provided a mounted PVC with local weights. Route all four AIC call sites through resolve_model_path() so locally-mounted weights are used instead of re-fetching from HuggingFace

Overview:

The profiler's resolve_model_path() helper components/src/dynamo/profiler/utils/profile_common.py was added to prefer a locally-mounted PVC model path over the HuggingFace model id when feeding model_path to aiconfigurator. It was wired into profile_sla.py only — four other call sites in thorough.py and rapid.py still passed the raw dgdr.model (the HF id), so aiconfigurator fetched config.json from huggingface.co even when the weights were already mounted in the profiler pod via modelCache.

Details:

resolve_model_path(dgdr) returns the local directory when modelCache resolves to a real directory, and falls back to dgdr.model.
Four call sites changed — each model_path=model becomes model_path=resolve_model_path(dgdr):

File Function AIC call
thorough.py run_thorough (Stage 1, enumeration) enumerate_profiling_configs(model_path=…)
thorough.py run_thorough (Stage 4, DGD generation) TaskConfig(model_path=…)
rapid.py _run_autoscale_sim TaskConfig(model_path=…)
rapid.py _run_default_sim build_default_task_configs(model_path=…)

Scope of the failure
The profiler passes the raw HF id, AIC issues a huggingface.co GET for config.json for any model that is neither a local directory nor in AIC's bundled DefaultHFModels set.

  • Gated model + No HF token → HTTP 401: Unauthorized.
  • Air-gapped cluster → the GET fails with a connection error — failed, even with a token.
  • Non-gated/gated model + HF token with internet -> succeeds but an avoidable round-trip every profile run even there's mounted PVC.
  • Model in DefaultHFModels → aiconfigurator serves a pre-bundled config → no GET, no failure, but the mounted PVC still goes unused.

With this fix, when modelCache resolves to a real directory aiconfigurator reads config.json from disk — no huggingface.co request.

Test:

Add components/src/dynamo/profiler/tests/unit/test_resolve_model_path.py — 16 unit tests

Where should the reviewer start?

  • components/src/dynamo/profiler/thorough.py
  • components/src/dynamo/profiler/rapid.py
  • components/src/dynamo/profiler/tests/unit/test_resolve_model_path.py

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

Summary by CodeRabbit

  • Bug Fixes

    • Improved model path resolution in RAPID and Thorough profiling so profiling uses local model mounts when present and falls back to remote model IDs otherwise.
  • Tests

    • Added comprehensive unit tests covering local vs remote model path selection and propagation through profiling workflows.

Review Change Stack

rapid.py and thorough.py passed the raw HF model id to AIC even when modelCache provided a mounted PVC with local weights. Route all four AIC call sites through resolve_model_path() so locally-mounted weights are used instead of re-fetching from HuggingFace

Signed-off-by: jooe0824 <jooe0824@sk.com>
@jooe0824
jooe0824 requested review from a team as code owners May 22, 2026 13:16
@copy-pr-bot

copy-pr-bot Bot commented May 22, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi jooe0824! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

@github-actions github-actions Bot added external-contribution Pull request is from an external contributor planner labels May 22, 2026
@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 44bd9bcb-ae3d-4716-bce6-beca4b62175b

📥 Commits

Reviewing files that changed from the base of the PR and between 65163f8 and a437d27.

📒 Files selected for processing (3)
  • components/src/dynamo/profiler/rapid.py
  • components/src/dynamo/profiler/tests/unit/test_resolve_model_path.py
  • components/src/dynamo/profiler/thorough.py

Walkthrough

Integrates resolve_model_path() into RAPID and THOROUGH profiling flows so model identifiers are resolved to local PVC paths when present (else HF IDs). Adds comprehensive unit tests that validate resolve_model_path() behavior and its propagation into rapid and thorough call sites.

Changes

Model path resolution across profiling

Layer / File(s) Summary
resolve_model_path function and core behavior tests
components/src/dynamo/profiler/tests/unit/test_resolve_model_path.py
Test module setup with helper functions to construct deployment request and model cache specs; unit tests for resolve_model_path() covering PVC directory resolution, HuggingFace ID fallback, path normalization, and edge cases (missing directories, file vs directory, incomplete cache fields).
RAPID strategy model path resolution
components/src/dynamo/profiler/rapid.py, components/src/dynamo/profiler/tests/unit/test_resolve_model_path.py
Import and usage of resolve_model_path(dgdr) in _run_autoscale_sim and _run_default_sim to replace model string argument with resolved path; integration tests verify propagation to TaskConfig.model_path and build_default_task_configs under mounted and non-mounted scenarios.
THOROUGH strategy model path resolution
components/src/dynamo/profiler/thorough.py, components/src/dynamo/profiler/tests/unit/test_resolve_model_path.py
Import and usage of resolve_model_path(dgdr) in run_thorough Stage 1 and Stage 4 to pass resolved path to enumerate_profiling_configs and TaskConfig construction; integration tests verify propagation under mounted and non-mounted scenarios.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: resolving local PVC model paths for AIC calls in the profiler.
Description check ✅ Passed The description covers all required sections with sufficient detail: overview, details with a change table, test additions, specific files for review, and related issues.
Docstring Coverage ✅ Passed Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

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

@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

Comment thread components/src/dynamo/profiler/rapid.py
Comment thread components/src/dynamo/profiler/rapid.py
Comment thread components/src/dynamo/profiler/rapid.py
jooe0824 and others added 7 commits May 23, 2026 00:19
Address PR review on the resolve_model_path change :

  - _generate_dgd_from_pick now resets ServiceConfig.model_path :
    served_model_path to dgdr.model before DGD generation, so a resolved local PVC path is not used as the deployment's served model name.
  - resolve_model_path now requires config.json in the PVC directory before returning the local path; otherwise it falls back to the HF id.
  Add tests for both fixes.

Signed-off-by: jooe0824 <jooe0824@sk.com>
@hhzhang16

Copy link
Copy Markdown
Contributor

/ok to test bd7cfc0

@hhzhang16
hhzhang16 merged commit 6f1bb83 into ai-dynamo:main May 26, 2026
73 checks passed
MartinRepo pushed a commit to MartinRepo/dynamo that referenced this pull request May 27, 2026
…9877)

Signed-off-by: jooe0824 <jooe0824@sk.com>
Co-authored-by: jooe0824 <jooe0824@sk.com>
Signed-off-by: Chi Xing <cxing@nvidia.com>
@jooe0824

jooe0824 commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for reviewing my PR! 😄
@hhzhang16 — I opened #10086, #10092 as a follow-up in the same area:
#10086 fixes a label collision in PickedParallelConfig.label() / make_parallel_label() that was silently corrupting thorough.py work_dir naming and AIC's groupby("parallel") dedup.
#10092 fixes autoscale behavior to prevent unintended worker rescaling during DGD generation.

Production change is ~12 lines, rest is regression tests. Would appreciate a review when you get a chance — thanks!

@jooe0824

Copy link
Copy Markdown
Contributor Author

👍 @hhzhang16 I also opened another PR as a follow-up in the same profiling area:
#10125 fixes decode sweep num_request range to prevent overshooting kv cache capacity.

Would appreciate a review when you get a chance - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contribution Pull request is from an external contributor fix planner size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants