-
Notifications
You must be signed in to change notification settings - Fork 664
refactor: standardize HF_CACHE to HF_HOME across codebase (use Hugging Face convention) #3310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughDocumentation references switched from HF_CACHE to HF_HOME. The container run script replaced HF_CACHE/DEFAULT_HF_CACHE with HF_HOME/DEFAULT_HF_HOME, updated option parsing to accept --hf-home|--hf-cache, adjusted defaulting, NONE handling, directory creation, volume mounting, and help text accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant RS as container/run.sh
participant ENV as Env Vars
participant DK as Docker
U->>RS: Invoke with args (--hf-home|--hf-cache, --workspace, etc.)
RS->>RS: Parse options (map --hf-cache|--hf-home -> HF_HOME)
RS->>ENV: Read HF_HOME / DEFAULT_HF_HOME
alt Workspace mounted and HF_HOME empty
RS->>RS: Set HF_HOME = DEFAULT_HF_HOME
end
alt HF_HOME == "NONE"
RS->>RS: Clear HF_HOME
else HF_HOME set
RS->>RS: mkdir -p $HF_HOME
RS->>DK: Run with volume: $HF_HOME -> /root/.cache/huggingface
end
RS-->>U: Container started with updated HF_HOME semantics
note over RS,Dk: All previous HF_CACHE references now map to HF_HOME
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
container/run.sh (1)
270-273: Consider quoting the variable in the volume mount for safety.While the current implementation works for typical paths, the
$HF_HOMEvariable in line 272 should be quoted to handle edge cases where the path might contain spaces.Apply this diff to add defensive quoting:
if [ -n "$HF_HOME" ]; then mkdir -p "$HF_HOME" - VOLUME_MOUNTS+=" -v $HF_HOME:/root/.cache/huggingface" + VOLUME_MOUNTS+=" -v \"$HF_HOME\":/root/.cache/huggingface" fi
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/backends/trtllm/multinode/multinode-examples.md(1 hunks)components/backends/trtllm/performance_sweeps/README.md(1 hunks)container/run.sh(5 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). (2)
- GitHub Check: sglang
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (7)
components/backends/trtllm/multinode/multinode-examples.md (1)
114-114: LGTM!Documentation updated correctly to reference HF_HOME instead of HF_CACHE, aligning with Hugging Face conventions.
components/backends/trtllm/performance_sweeps/README.md (1)
77-77: LGTM!Documentation updated correctly to reference HF_HOME, maintaining consistency with the first documentation file.
container/run.sh (5)
34-35: LGTM!Safe parameter expansion syntax correctly used. The variable initialization properly defaults to an empty string if HF_HOME is unset, and the default path is appropriately updated.
88-94: LGTM!Excellent backward compatibility approach. The combined option flag
--hf-cache|--hf-homeensures existing scripts using--hf-cachecontinue to work while encouraging migration to the standardized--hf-home.
253-255: LGTM!Defaulting logic correctly updated to use HF_HOME. When workspace is mounted and HF_HOME is not set, it appropriately defaults to DEFAULT_HF_HOME.
266-268: LGTM!NONE handling correctly updated. Setting
--hf-home NONEappropriately clears the HF_HOME variable to prevent mounting.
322-322: LGTM!Help text accurately updated to reflect the combined
--hf-home|--hf-cacheoption and correctly describes the new semantics.
- Update container/run.sh to use HF_HOME variable and --hf-home option - Maintain backward compatibility with --hf-cache option - Update documentation references in multinode and performance sweep examples - Align with Hugging Face standard environment variable naming Ref: https://github.com/search?q=repo%3Aai-dynamo%2Fdynamo%20HF_HOME&type=code Signed-off-by: Keiven Chang <[email protected]>
bb96ce1 to
7bc3c8b
Compare
…g Face convention) (#3310) Signed-off-by: Keiven Chang <[email protected]>
Overview:
This PR standardizes the use of
HF_HOMEinstead ofHF_CACHEacross the codebase to align with Hugging Face's standard environment variable naming conventions. The changes maintain backward compatibility while updating the container run script and documentation.Details:
container/run.shto useHF_HOMEvariable instead ofHF_CACHE--hf-homecommand line option while maintaining--hf-cachefor backward compatibility${HF_HOME:-}parameter expansion for safe environment variable initializationHF_HOMEWhere should the reviewer start?
container/run.sh- Main changes to variable names, command line options, and logiccomponents/backends/trtllm/multinode/multinode-examples.md- Documentation updatecomponents/backends/trtllm/performance_sweeps/README.md- Documentation update/coderabbit profile chill
Summary by CodeRabbit
New Features
Documentation