Skip to content

Conversation

@keivenchang
Copy link
Contributor

@keivenchang keivenchang commented Sep 27, 2025

Overview:

This PR fixes the HF cache mount point in the container run script to dynamically determine the correct user directory based on the target type.

Details:

  • Dynamic cache mount point: Modified container/run.sh to use /home/ubuntu/.cache/huggingface for local-dev targets and /root/.cache/huggingface for dev/production targets
  • Backward compatibility: Maintains existing behavior for all non-local-dev targets while fixing the issue for local-dev environments

Where should the reviewer start?

Review the changes in container/run.sh around lines 270-279, specifically the logic that determines HF_CACHE_TARGET based on the target type.

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

/coderabbit profile chill

Summary by CodeRabbit

  • Bug Fixes
    • Container now auto-detects environment and mounts the Hugging Face cache to the appropriate path, improving compatibility between local-dev and other images.
    • Preserves the existing host cache location and ensures directories are created before mounting, reducing first-run failures and permission errors.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 27, 2025

Walkthrough

Updates container/run.sh to conditionally set the Docker bind mount target for the Hugging Face cache based on TARGET or IMAGE name, switching between /home/ubuntu/.cache/huggingface for local-dev and /root/.cache/huggingface otherwise. Maintains host-side $HF_CACHE and ensures directory creation before mounting.

Changes

Cohort / File(s) Summary
Mount target selection in run script
container/run.sh
Replace fixed HF cache mount target with conditional path: if TARGET=local-dev or IMAGE contains "local-dev" → mount to /home/ubuntu/.cache/huggingface; else → /root/.cache/huggingface. Preserve host $HF_CACHE and pre-create directories before docker run.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as User
  participant S as run.sh
  participant D as Docker

  U->>S: Execute run.sh
  S->>S: Check TARGET / IMAGE for "local-dev"
  alt local-dev
    S->>S: Set mount target=/home/ubuntu/.cache/huggingface
  else non-local
    S->>S: Set mount target=/root/.cache/huggingface
  end
  S->>S: Ensure host $HF_CACHE exists
  S->>D: docker run -v $HF_CACHE:target ...
  D-->>U: Container starts with HF cache mounted
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I hopped to the docks with a cache in tow,
Switching paths where the breezes blow.
Root burrow here, ubuntu there—
Local dev clues in the name we wear.
Mounts aligned, I thump in delight,
Carrots cached, containers light. 🥕🐇

Pre-merge checks

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The description includes Overview, Details, and Reviewer Start sections as per the template, but the Related Issues section does not use a valid action keyword with an actual issue reference and instead contains an unrelated slash command, making it non-compliant with the required format. Please update the Related Issues section to use a proper GitHub action keyword such as “Closes,” “Fixes,” or “Resolves” followed by a valid issue number, and remove or replace the incorrect “/coderabbit profile chill” entry.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly and accurately describes the core change of dynamically adjusting the HF cache mount point based on the deployment target, follows the conventional prefix “fix:” and remains clear and concise without unnecessary detail.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9d73be1 and 8c0a4fc.

📒 Files selected for processing (1)
  • container/run.sh (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: keivenchang
PR: ai-dynamo/dynamo#3035
File: lib/runtime/src/metrics/prometheus_names.rs:49-53
Timestamp: 2025-09-16T00:26:37.092Z
Learning: keivenchang prefers consistency in metric naming standardization over strict adherence to Prometheus conventions about gauge vs counter suffixes. When standardizing metrics naming, prioritize consistency across the codebase rather than technical pedantry about individual metric type conventions.
⏰ 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). (1)
  • GitHub Check: Build and Test - dynamo

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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.

@keivenchang keivenchang force-pushed the keivenchang/run.sh-fix-for-local-dev branch from 8c0a4fc to 20e0c39 Compare September 30, 2025 03:32
- Use /home/ubuntu/.cache/huggingface for local-dev targets
- Use /root/.cache/huggingface for dev/production targets
- Check both TARGET variable and IMAGE name for local-dev detection

Signed-off-by: Keiven Chang <[email protected]>
@keivenchang keivenchang merged commit 8c88434 into main Sep 30, 2025
17 of 18 checks passed
@keivenchang keivenchang deleted the keivenchang/run.sh-fix-for-local-dev branch September 30, 2025 15:50
keivenchang added a commit that referenced this pull request Sep 30, 2025
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.

4 participants