Skip to content

Conversation

biswapanda
Copy link
Contributor

@biswapanda biswapanda commented Oct 15, 2025

Overview:

When running the perf job with the specified command, the script fails due to an incorrect repo id format error during the tokenizer creation process. The expected result is for the perf job to finish successfully.

closes: DEP-526

Details:

Where should the reviewer start?

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

  • closes GitHub issue: #xxx

Summary by CodeRabbit

  • Refactor

    • Switched to a more efficient model download method to improve speed and reliability.
    • Pinned the model version to ensure consistent behavior across deployments.
  • Chores

    • Simplified setup by removing the need for a token-based configuration.
    • Standardized the model storage location for easier management and persistence across runs.

@biswapanda biswapanda self-assigned this Oct 15, 2025
@biswapanda biswapanda requested review from a team as code owners October 15, 2025 04:47
Copy link
Contributor

coderabbitai bot commented Oct 15, 2025

Walkthrough

Replaces Hugging Face CLI + HF_TOKEN–based model download with an hf-transfer flow. Introduces HF_HOME, HF_HUB_ENABLE_HF_TRANSFER, and MODEL_REVISION. Pins download to a specific revision. Changes storage path and volume mount to /model-store. Removes secretKeyRef for HF_TOKEN and updates the download command and working directory.

Changes

Cohort / File(s) Summary
Model cache workflow
recipes/llama-3-70b/model-cache/model-download.yaml
Switch from huggingface-cli with HF_TOKEN to hf download using hf-transfer; set HF_HOME=/model-store, enable HF transfer, add MODEL_REVISION pin; change workingDir and volume mount to /model-store; remove HF_TOKEN secret and secretKeyRef.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant J as K8s Job/Container
  participant HF as Hugging Face Hub
  participant FS as /model-store (Volume)

  Note over J: Env: HF_HOME=/model-store<br/>HF_HUB_ENABLE_HF_TRANSFER=1<br/>MODEL_REVISION=<pin>
  J->>FS: Set working directory to /model-store
  J->>HF: hf download <model> --revision $MODEL_REVISION
  HF-->>J: Stream artifacts (hf-transfer)
  J->>FS: Write model files to /model-store
  Note over J,FS: No HF_TOKEN secret used
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I hopped to the hub with a whisk of my ear,
No secrets in pockets—just paths crystal clear.
A pinned little revision, a speedy transfer cheer,
Models in /model-store, neatly landing here.
Thump-thump: cache is tidy, the carrots are near! 🥕✨

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description includes the required section headings but leaves the Details and Where should the reviewer start sections entirely blank with placeholder comments, and the Related Issues section uses a placeholder “#xxx” rather than an actual issue reference. The Overview also misplaces “closes: DEP-526” instead of reserving the Related Issues section for issue closure. These omissions mean the description does not fully adhere to the repository template or provide sufficient context about the changes. Please fill in the Details section with a clear summary of the actual code and configuration changes, specify which files or sections the reviewer should inspect under “Where should the reviewer start?”, correct the Related Issues entry to reference the actual issue number, and remove or relocate the misplaced “closes: DEP-526” from the Overview.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title “fix: update model path” refers to a genuine aspect of the changeset by highlighting the updated storage location, but it omits the more substantial shift to the HF-transfer download flow and related environment variable adjustments, making it only partially descriptive of the main changes.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

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.

@github-actions github-actions bot added the fix label Oct 15, 2025
@biswapanda
Copy link
Contributor Author

/ok to test 9f3f7ed

@grahamking
Copy link
Contributor

Could you extend the title from "update model path" to be more specific? Which model path(s)?

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.

2 participants