Pinning liger-kernal version #9
Conversation
WalkthroughAdded an optional CUDA dependency Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pyproject.toml (3)
49-49: Tighten the “pin”: use a bounded range (or exact pin) and annotate the reason.If the intent is to lock the working fix, prefer >=0.5.10,<0.5.11 (or ==0.5.10) to avoid future breakage; also add a brief comment referencing the upstream issue.
Apply one of the following:
Option A (bounded minor):
- "liger-kernel>=0.5.10" + "liger-kernel>=0.5.10,<0.5.11", # Fix Gemma _CONFIG_FOR_DOC import error; see HF TRL issue 3480Option B (exact pin for apps):
- "liger-kernel>=0.5.10" + "liger-kernel==0.5.10", # Fix Gemma _CONFIG_FOR_DOC import error; see HF TRL issue 3480
39-47: Duplicate dependency: filelock listed twice with different minima.You have both filelock>=3.0 (Line 39) and filelock>=3.19.1 (Line 46). Keep one, preferably the stricter one.
Apply this diff:
- "filelock>=3.0", @@ - "filelock>=3.19.1", + "filelock>=3.19.1",
48-50: Confirm whether liger-kernel must be core or should live under a CUDA/kernels extra.If Training Hub flows exist that don’t require liger-kernel, consider moving it to [project.optional-dependencies].cuda (or a new [project.optional-dependencies].kernels) to reduce base install weight. Otherwise, keeping it core is fine.
I can draft the extras rewire and minimal import smoke test for CI to validate Gemma import. Want me to push that?
Maxusmusti
left a comment
Maxusmusti
left a comment
There was a problem hiding this comment.
Could you move this to the cuda dependencies, rather than the base dependencies? This package should be installed when installing via pip install training-hub[cuda].
Fiona-Waters
force-pushed
the
liger-kernel
branch
from
7086176 to
870674a
Compare
Fiona-Waters
force-pushed
the
liger-kernel
branch
from
870674a to
d0815f2
Compare
Updated. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyproject.toml (2)
39-47: Duplicate dependency: filelock listed twice with conflicting bounds.Keep the stricter one once to avoid resolver ambiguity.
Apply this diff:
@@ - "filelock>=3.0", @@ - "filelock>=3.19.1", + "filelock>=3.19.1",
45-45: Replace 'attr' with 'attrs' in pyproject.tomlPyPI shows 'attr' is a different project (v0.3.2); the intended runtime library is 'attrs' (latest v25.3.0). Update pyproject.toml (line 45): replace
"attr>=0.3.2"
with
"attrs>=23.2.0"
🧹 Nitpick comments (1)
pyproject.toml (1)
14-16: Remove build tools from runtime dependencies (setuptools, wheel).They’re already in [build-system] and shouldn’t be required at runtime.
Apply this diff:
@@ - "setuptools>=80.0", @@ - "wheel>=0.43",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyproject.toml(1 hunks)
🔇 Additional comments (3)
pyproject.toml (3)
48-49: Do not ship pytest as a runtime dependency; move to dev extra.Keep pytest out of end-user installs; include it under [project.optional-dependencies].dev.
Apply this diff:
@@ - "pytest>=8.0" + "pytest>=8.0", @@ dev = [ "ipykernel", - "ipython" + "ipython", + "pytest>=8.0" ]And remove pytest from [project].dependencies:
@@ - "pytest>=8.0"
32-37: Constraints are satisfiable — PyPI publishes 2025+ releasesfsspec latest: 2025.9.0; regex latest: 2025.9.18 — the >=2025.0 floors are satisfiable.
61-62: LGTM: CUDA extra now pins liger-kernel to a fixed-good range.Sandbox couldn't fetch PyPI metadata (SSL certificate verification failed), so verification couldn't be completed here — confirm instructlab/instructlab-training doesn't force a conflicting liger-kernel version and that both CPU- and CUDA-only Training Hub images resolve correctly.
2 participants
While working on creating a runtime image that encorporates kubeflow training and training hub related dependencies I came across the following error when using the image to run osft_llama_example.py and osft_llama_example.py:
Updating the liger-kernel dependency to 0.5.10 fixes this issue as per huggingface/trl#3480 . This is an instruct-lab dependency so not sure where would be best to fix it.
Summary by CodeRabbit