Conversation
📝 WalkthroughWalkthroughThis change updates the installation instructions and references for the Changes
Estimated code review effort1 (<10 minutes) Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scripts/cutcrossentropy_install.py (1)
30-33: Centralise the commit hash to avoid silent drift between call-sitesThe hard-coded
631d646appears in several places (this script,README.md,__init__.py). In the past we’ve had mismatches when only one file was updated. A single constant will make future upgrades trivial and error-free.+# Pin for Axolotl’s fork of cut-cross-entropy +CCE_COMMIT = "631d646" ... -print( - UNINSTALL_PREFIX - + f'{UV_PREFIX}pip install "cut-cross-entropy[transformers] @ git+https://github.com/axolotl-ai-cloud/ml-cross-entropy.git@631d646"' -) +install_cmd = ( + UNINSTALL_PREFIX + + f'{UV_PREFIX}pip install "cut-cross-entropy[transformers] ' + + f'@ git+https://github.com/axolotl-ai-cloud/ml-cross-entropy.git@{CCE_COMMIT}"' +) +print(install_cmd)Consider exposing
CCE_COMMITfrom a small helper module so docs and other code can import the same value.src/axolotl/integrations/cut_cross_entropy/__init__.py (1)
35-38: DRY the install message to share the commit constant
_CCE_INSTALL_MESSAGEre-embeds the commit string, duplicating knowledge already present inscripts/cutcrossentropy_install.pyand the README. Import the commit from a shared module (or define a localCCE_COMMITconstant) and build the message dynamically:+# Pin for Axolotl’s fork of cut-cross-entropy +CCE_COMMIT = "631d646" _CCE_INSTALL_MESSAGE = ( "Please install Axolotl's fork of cut_cross_entropy with transformers support using " - '`pip install "cut-cross-entropy[transformers] @ git+https://github.com/axolotl-ai-cloud/ml-cross-entropy.git@631d646"`' + f'`pip install "cut-cross-entropy[transformers] ' + f'@ git+https://github.com/axolotl-ai-cloud/ml-cross-entropy.git@{CCE_COMMIT}"`' )This keeps the string in exactly one place and prevents future inconsistencies.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/colab-notebooks/colab-axolotl-example.ipynb(1 hunks)scripts/cutcrossentropy_install.py(1 hunks)src/axolotl/integrations/cut_cross_entropy/README.md(1 hunks)src/axolotl/integrations/cut_cross_entropy/__init__.py(1 hunks)
🔇 Additional comments (2)
src/axolotl/integrations/cut_cross_entropy/README.md (1)
22-23: LGTM – hash updated consistentlyDocumentation now points at the same commit used by the install script.
examples/colab-notebooks/colab-axolotl-example.ipynb (1)
42-44: Verify the new commit hash actually exists on the remote before mergingSwitching to a hard-pinned SHA keeps the notebook reproducible, but if the commit
631d646is force-pushed or GC’d, the Colab install step will break.
Please double-check the hash is present onaxolotl-ai-cloud/ml-cross-entropyand ideally tag it (or cut a release) so future users aren’t stranded.
|
📖 Documentation Preview: https://687e44a823c62c6811f2b542--resonant-treacle-0fd729.netlify.app Deployed on Netlify from commit 29491cc |
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
2 participants
Description
Motivation and Context
How has this been tested?
Screenshots (if appropriate)
Types of changes
Social Handles (Optional)
Summary by CodeRabbit