fix: Support local filesystem paths in model download/discovery#7390
fix: Support local filesystem paths in model download/discovery#7390dmvevents wants to merge 1 commit intoai-dynamo:mainfrom
Conversation
When a TRT-LLM worker registers with --model-path /shared/path/to/model, the frontend's download_config() passes the local path to from_hf(), which treats it as a HuggingFace repo name and fails with HFValidationError. This fix adds an early check in from_hf(): if the path is an absolute filesystem path that exists, return it directly without attempting any HuggingFace download. This enables models stored on local/shared filesystems (FSx Lustre, NFS, etc.) to be served without requiring a HuggingFace repo. Fixes part of ai-dynamo#7389 Signed-off-by: Anton <antonai@users.noreply.github.com>
|
👋 Hi dmvevents! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
bot
added
external-contribution
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip You can get early access to new features in CodeRabbit.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/llm/src/hub.rs (1)
80-80:⚠️ Potential issue | 🟠 MajorPlease rebase this PR against main before merge.
Currently 1999 commits behind main, which far exceeds the recommended threshold of ~25 commits. Use
git rebase origin/mainto sync with latest changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/llm/src/hub.rs` at line 80, The branch is far behind main and must be rebased before merging; checkout the branch containing lib/llm/src/hub.rs (where from_hf is defined), run git fetch origin && git rebase origin/main, resolve any conflicts (especially around changes to from_hf or nearby functions), run the test suite/lints, then force-push the rebased branch to update the PR.
🧹 Nitpick comments (1)
lib/llm/src/hub.rs (1)
84-90: Add a focused test for the new absolute-local-path fast path.This behavior is core to the fix and currently has no direct unit coverage in this file. A targeted test would prevent regressions.
🧪 Example test addition
#[cfg(test)] mod tests { use super::*; @@ + #[tokio::test] + async fn test_from_hf_existing_absolute_path_returns_directly() { + let path = std::env::temp_dir().join(format!("dynamo-local-model-{}", std::process::id())); + std::fs::create_dir_all(&path).unwrap(); + + let resolved = from_hf(&path, true).await.unwrap(); + assert_eq!(resolved, path); + + let _ = std::fs::remove_dir_all(&path); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/llm/src/hub.rs` around lines 84 - 90, Add a unit test that exercises the absolute-local-path fast path in lib/llm/src/hub.rs: create a real temporary absolute path on disk and pass it as name (the variable used in the diff) and assert the function returns that same PathBuf (the code path that does return Ok(name.to_path_buf())); the test should call the same public function in hub.rs that resolves model locations and verify the returned path equals the provided absolute path and that no download logic is triggered (e.g., by not relying on network or by asserting no downstream downloader was invoked).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/llm/src/hub.rs`:
- Around line 87-89: Replace the current absolute-path existence check that uses
name.exists() with a non-panicking IO-aware check: call name.try_exists() and
handle the Result, and only treat the path as a local model if try_exists()
returns Ok(true) and name.is_dir() is true; if try_exists() returns an Err
propagate or log the IO error appropriately so it doesn't silently fall through
to HF download, and ensure this change is applied to the branch that logs "Using
local model path: {model_name}" before returning the PathBuf (this affects the
code handling name, model_name and downstream update_dir()).
---
Outside diff comments:
In `@lib/llm/src/hub.rs`:
- Line 80: The branch is far behind main and must be rebased before merging;
checkout the branch containing lib/llm/src/hub.rs (where from_hf is defined),
run git fetch origin && git rebase origin/main, resolve any conflicts
(especially around changes to from_hf or nearby functions), run the test
suite/lints, then force-push the rebased branch to update the PR.
---
Nitpick comments:
In `@lib/llm/src/hub.rs`:
- Around line 84-90: Add a unit test that exercises the absolute-local-path fast
path in lib/llm/src/hub.rs: create a real temporary absolute path on disk and
pass it as name (the variable used in the diff) and assert the function returns
that same PathBuf (the code path that does return Ok(name.to_path_buf())); the
test should call the same public function in hub.rs that resolves model
locations and verify the returned path equals the provided absolute path and
that no download logic is triggered (e.g., by not relying on network or by
asserting no downstream downloader was invoked).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 909d6019-2f38-4d41-b578-b0f84345afba
📒 Files selected for processing (1)
lib/llm/src/hub.rs
| if name.is_absolute() && name.exists() { | ||
| tracing::info!("Using local model path: {model_name}"); | ||
| return Ok(name.to_path_buf()); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Rust std::path::Path, how do exists()andtry_exists()differ, and doesexists() return false on permission-denied errors?
💡 Result:
Path::exists(&self) -> boolreturnstrueonly if the path exists and its metadata is accessible; it coerces any error tofalse(including permission errors and broken symlinks). So yes:exists()can returnfalseon “permission denied”. [1]Path::try_exists(&self) -> io::Result<bool>distinguishes “does not exist” from “couldn’t be determined”: it returnsOk(true)if it verified the path exists,Ok(false)if it verified it doesn’t (including broken symlinks), andErr(e)when existence can’t be confirmed/denied (e.g., permission denied on a parent directory). [1]
In other words, exists() is essentially “try_exists() but swallow errors as false.” [1]
Sources
[1] Rust standard library docs: std::path::Path::{exists, try_exists}. (doc.rust-lang.org)
Citations:
🏁 Script executed:
# First, verify the exact code at lines 87-89 in lib/llm/src/hub.rs
echo "=== Checking lib/llm/src/hub.rs around lines 87-89 ==="
if [ -f "lib/llm/src/hub.rs" ]; then
sed -n '80,95p' lib/llm/src/hub.rs
else
echo "File not found at lib/llm/src/hub.rs"
fiRepository: ai-dynamo/dynamo
Length of output: 904
🏁 Script executed:
# Check the broader context of the from_hf function
echo "=== Function context ==="
if [ -f "lib/llm/src/hub.rs" ]; then
sed -n '75,120p' lib/llm/src/hub.rs
fiRepository: ai-dynamo/dynamo
Length of output: 2038
🏁 Script executed:
# Verify the downstream usage in model_card.rs
echo "=== Checking lib/llm/src/model_card.rs around lines 460-476 ==="
if [ -f "lib/llm/src/model_card.rs" ]; then
sed -n '455,480p' lib/llm/src/model_card.rs
fiRepository: ai-dynamo/dynamo
Length of output: 1113
🏁 Script executed:
# Search for all callers of from_hf() to understand the flow
echo "=== Searching for callers of from_hf ==="
rg "from_hf\(" -A 2 -B 2 lib/Repository: ai-dynamo/dynamo
Length of output: 1436
Use try_exists() and validate the local path is a directory.
Path::exists() masks IO errors (permission denied, permission issues on shared filesystems) and returns false, potentially causing valid local paths to silently fall through to HF downloads. Additionally, this branch accepts any existing path including files, but downstream update_dir() expects a directory.
💡 Proposed fix
- if name.is_absolute() && name.exists() {
- tracing::info!("Using local model path: {model_name}");
- return Ok(name.to_path_buf());
- }
+ if name.is_absolute() {
+ match name.try_exists() {
+ Ok(true) if name.is_dir() => {
+ tracing::info!("Using local model path: {model_name}");
+ return Ok(name.to_path_buf());
+ }
+ Ok(true) => {
+ anyhow::bail!("Local model path must be a directory: {model_name}");
+ }
+ Ok(false) => {}
+ Err(e) => {
+ anyhow::bail!("Cannot access local model path '{model_name}': {e}");
+ }
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if name.is_absolute() && name.exists() { | |
| tracing::info!("Using local model path: {model_name}"); | |
| return Ok(name.to_path_buf()); | |
| if name.is_absolute() { | |
| match name.try_exists() { | |
| Ok(true) if name.is_dir() => { | |
| tracing::info!("Using local model path: {model_name}"); | |
| return Ok(name.to_path_buf()); | |
| } | |
| Ok(true) => { | |
| anyhow::bail!("Local model path must be a directory: {model_name}"); | |
| } | |
| Ok(false) => {} | |
| Err(e) => { | |
| anyhow::bail!("Cannot access local model path '{model_name}': {e}"); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/llm/src/hub.rs` around lines 87 - 89, Replace the current absolute-path
existence check that uses name.exists() with a non-panicking IO-aware check:
call name.try_exists() and handle the Result, and only treat the path as a local
model if try_exists() returns Ok(true) and name.is_dir() is true; if
try_exists() returns an Err propagate or log the IO error appropriately so it
doesn't silently fall through to HF download, and ensure this change is applied
to the branch that logs "Using local model path: {model_name}" before returning
the PathBuf (this affects the code handling name, model_name and downstream
update_dir()).
|
I want to know when we can merge |
|
@OAMchronicle — The PR is ready from our end. The CodeRabbit review from March 14 left some comments — are those addressed from your perspective? If there are any remaining items, happy to resolve them. Otherwise this should be ready for final approval and merge. |
No, that wasn't me. I've already found a solution, though I'm not sure if it's the same problem you encountered. |
2 participants
Summary
--model-path /shared/path/to/model, the frontend's model discovery callsfrom_hf()which passes the local path to the HuggingFace provider, causing a validation errorProblem
Fix
In
lib/llm/src/hub.rs::from_hf():Test plan
Related issue: #7389
Summary by CodeRabbit