fix: update model express common to use HF_HOME (if specified) #3638
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughAdded environment-variable-based cache resolution in get_model_express_cache_dir to check HF_HUB_CACHE and HF_HOME (HF_HOME joined with "hub"), preserving existing fallbacks. Added a test validating HF_HOME handling. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Hub as hub::get_model_express_cache_dir
participant Env as Environment
participant FS as Filesystem
Caller->>Hub: get_model_express_cache_dir()
Hub->>Env: read HF_HUB_CACHE
alt HF_HUB_CACHE set
Hub-->>Caller: return HF_HUB_CACHE
else HF_HUB_CACHE not set
Hub->>Env: read HF_HOME
alt HF_HOME set
Hub->>FS: join(HF_HOME, "hub")
Hub-->>Caller: return HF_HOME/hub
else HF_HOME not set
Hub->>FS: resolve existing fallbacks<br/>(MODEL_EXPRESS_CACHE_PATH, HOME/USERPROFILE)
Hub-->>Caller: return fallback path
end
end
Note over Hub: Env var precedence: HF_HUB_CACHE → HF_HOME/hub → existing fallbacks
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
biswapanda
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/llm/src/hub.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: sglang
- GitHub Check: vllm (amd64)
- GitHub Check: Build and Test - dynamo
- GitHub Check: tests (lib/runtime/examples)
- GitHub Check: clippy (launch/dynamo-run)
- GitHub Check: tests (lib/bindings/python)
- GitHub Check: clippy (.)
- GitHub Check: clippy (lib/bindings/python)
- GitHub Check: tests (launch/dynamo-run)
🔇 Additional comments (1)
lib/llm/src/hub.rs (1)
93-97: LGTM!The documentation comments improve code clarity and reference the official Hugging Face documentation.
Comment on lines
+99
to
+103
| // Check HF_HOME environment variable (standard Hugging Face cache directory) | ||
| // reference: https://huggingface.co/docs/huggingface_hub/en/package_reference/environment_variables#hfhome | ||
| if let Ok(hf_home) = env::var("HF_HOME") { | ||
| return PathBuf::from(hf_home).join("hub"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the precedence order across all cache path environment variables.
The implementation correctly appends "hub" to HF_HOME per the Hugging Face documentation. However, ensure that the precedence order (HF_HUB_CACHE → HF_HOME → MODEL_EXPRESS_CACHE_PATH → HOME fallback) aligns with expected behavior and is thoroughly tested.
Run the following script to check if any existing code or documentation assumes a different precedence order:
🏁 Script executed:
#!/bin/bash
# Description: Search for references to cache environment variables and their precedence
# Search for any documentation or comments about cache precedence
rg -n -C3 'HF_HUB_CACHE|HF_HOME|MODEL_EXPRESS_CACHE_PATH' --type rust --type md
# Search for other usages of these environment variables
rg -n 'env::var.*HF_HUB_CACHE|env::var.*HF_HOME|env::var.*MODEL_EXPRESS_CACHE_PATH' --type rustLength of output: 4246
Add tests covering all cache path environment variable branches and their precedence
- Test that
HF_HUB_CACHEoverridesHF_HOMEandMODEL_EXPRESS_CACHE_PATH. - Test that
MODEL_EXPRESS_CACHE_PATHis used whenHF_HUB_CACHEandHF_HOMEare unset. - Test fallback behavior to
$HOME/.cache/huggingface/hubwhen none are set.
🤖 Prompt for AI Agents
In lib/llm/src/hub.rs around lines 99–103, add unit tests exercising all
environment-variable branches and their precedence: write three tests that (1)
set HF_HUB_CACHE and also set HF_HOME and MODEL_EXPRESS_CACHE and assert the
function returns HF_HUB_CACHE, (2) unset HF_HUB_CACHE but set
MODEL_EXPRESS_CACHE (and unset HF_HOME) and assert it returns
MODEL_EXPRESS_CACHE, and (3) unset HF_HUB_CACHE and MODEL_EXPRESS_CACHE and set
HOME to a temp directory and assert it falls back to
$HOME/.cache/huggingface/hub. In each test manipulate env vars with
std::env::set_var/remove_var (saving and restoring originals) or use a helper to
ensure isolation, use a tempdir for expected paths, and run tests serially
(e.g., serial_test or single-threaded test runner) so global env changes don’t
interfere. Ensure assertions compare PathBufs and clean up/restore env state at
test end.
Comment on lines
+134
to
+145
| #[test] | ||
| fn test_get_model_express_cache_dir_with_hf_home() { | ||
| // Test that HF_HOME is respected when set | ||
| unsafe { | ||
| env::set_var("HF_HOME", "/custom/cache/path"); | ||
| let cache_dir = get_model_express_cache_dir(); | ||
| assert_eq!(cache_dir, PathBuf::from("/custom/cache/path/hub")); | ||
|
|
||
| // Clean up | ||
| env::remove_var("HF_HOME"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Environment variable mutation in tests causes race conditions.
Using env::set_var in an unsafe block is problematic because Rust runs tests concurrently by default. This can cause:
- Race conditions: Multiple tests modifying environment variables simultaneously
- Flaky tests: Non-deterministic failures when tests interfere with each other
- Incomplete isolation: The test doesn't clear
HF_HUB_CACHEorMODEL_EXPRESS_CACHE_PATH, so if those are set in the environment, this test won't actually exercise theHF_HOMEcode path
Consider one of these solutions:
Solution 1 (Recommended): Use the serial_test crate
Add to Cargo.toml:
[dev-dependencies]
serial_test = "3.0"Then apply this diff:
+ #[serial_test::serial]
#[test]
fn test_get_model_express_cache_dir_with_hf_home() {
- // Test that HF_HOME is respected when set
unsafe {
+ // Clear other cache env vars to ensure HF_HOME is tested
+ env::remove_var("HF_HUB_CACHE");
+ env::remove_var("MODEL_EXPRESS_CACHE_PATH");
+
env::set_var("HF_HOME", "/custom/cache/path");
let cache_dir = get_model_express_cache_dir();
assert_eq!(cache_dir, PathBuf::from("/custom/cache/path/hub"));
// Clean up
env::remove_var("HF_HOME");
}
}Solution 2: Refactor to accept environment as a parameter
Refactor get_model_express_cache_dir to accept an environment provider trait, allowing tests to inject mock environments without mutating global state.
🤖 Prompt for AI Agents
In lib/llm/src/hub.rs around lines 134-145, the test mutates process-wide env
vars unsafely causing race conditions; fix it by adding serial_test as a
dev-dependency in Cargo.toml and annotating the test with serial_test::serial so
it runs single-threaded, remove the unsafe block and use
std::env::set_var/remove_var normally inside the test, and ensure you clear or
save/restore HF_HUB_CACHE and MODEL_EXPRESS_CACHE_PATH (unset them or save then
restore) before setting HF_HOME so the test exercises the HF_HOME path in
isolation and cleans up all modified env vars at the end.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview:
respect
HF_HOMEenv var if specified.https://huggingface.co/docs/huggingface_hub/en/package_reference/environment_variables#hfhome
HF_HOMEin needed in long term because HF moved thexetfolder underHF_HOMEcloses DYN-1248
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Tests