Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request includes two separate fixes: updating a checkpoint filename constant from an incomplete path to the complete Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/axolotl/core/trainers/base.py (1)
54-54: Consider consolidating duplicate constant definitions.
TOKENS_STATE_FILEis defined independently in bothbase.pyandtokens_per_second.py. This duplication creates a risk that future changes might update one but not the other, breaking checkpoint resumption.Consider defining the constant in one location (e.g., a shared constants module) and importing it in both files to maintain consistency.
♻️ Potential consolidation approach
Create a shared constants module (e.g.,
src/axolotl/core/trainers/constants.py):"""Shared constants for trainers and callbacks.""" TOKENS_STATE_FILE = "tokens_state.json"Then import in both files:
+from axolotl.core.trainers.constants import TOKENS_STATE_FILE + -TOKENS_STATE_FILE = "tokens_state.json"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/axolotl/core/trainers/base.py` at line 54, TOKENS_STATE_FILE is duplicated across modules; consolidate it into a single shared constant and import it where needed: create a small constants module (e.g., trainers.constants with TOKENS_STATE_FILE = "tokens_state.json"), then replace the local TOKENS_STATE_FILE definitions in base.py and tokens_per_second.py to import TOKENS_STATE_FILE from that new module so both trainers use the same symbol and avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/axolotl/core/trainers/base.py`:
- Line 54: TOKENS_STATE_FILE is duplicated across modules; consolidate it into a
single shared constant and import it where needed: create a small constants
module (e.g., trainers.constants with TOKENS_STATE_FILE = "tokens_state.json"),
then replace the local TOKENS_STATE_FILE definitions in base.py and
tokens_per_second.py to import TOKENS_STATE_FILE from that new module so both
trainers use the same symbol and avoid drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b1e048c-5d53-40ff-a639-3b748884cf14
📒 Files selected for processing (2)
src/axolotl/core/trainers/base.pysrc/axolotl/loaders/tokenizer.py
|
📖 Documentation Preview: https://69bd4611a3243b3a8c53001a--resonant-treacle-0fd729.netlify.app Deployed on Netlify from commit 25caf42 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
related details https://github.com/axolotl-ai-cloud/axolotl/actions/runs/23338977708/job/67889510691 |
Description
These bugs were showing up in CI.
Summary by CodeRabbit