Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Update project description to be architecture-agnostic - Remove numpy from base deps (transitive via flax) - Move pytest back to base deps (needed everywhere) - Remove unused torchvision dependency - Remove non-existent jax[gpu] and flash-linear-attention[gpu] extras - Remove unused profile optional-dependency group - Remove torchvision from uv sources - Switch CI GPU tests to uv sync - Complete tpuv7 K8s Pod uv setup flow - Update CLAUDE.md install commands Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe pull request consolidates dependency management and CI setup to use Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Code Review
This pull request streamlines the project's dependency management and CI/CD workflows by adopting uv sync and uv run. Key changes include updating pyproject.toml to refine dependency groups, removing unused packages like torchvision, and updating the TPU launch script and GPU test configuration. Feedback highlights that moving pytest to the base dependencies is non-standard for a library. Additionally, removing the [gpu] extra from flash-linear-attention may cause issues with Triton-based kernels, and the TPU launch script needs to use uv run to correctly access the virtual environment created by uv sync.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/launch-tpuv7.yml (1)
36-39: The pip fallback may not work as expected after a faileduv sync.If
uv sync --extra tpufails partway through (e.g., due to a dependency resolution error), it may have created a partial.venvdirectory. The subsequentpip install -e '.[tpu]'runs against the system Python, not the venv, which could lead to an inconsistent state where dependencies are split between system and venv.♻️ Consider a cleaner fallback strategy
# 4. Clone and setup project git clone git@github.com:primatrix/pallas-kernel.git ~/pallas-kernel cd ~/pallas-kernel - uv sync --extra tpu || pip install -e '.[tpu]' + if ! uv sync --extra tpu; then + echo "uv sync failed, falling back to pip" + rm -rf .venv # Clean up partial state + pip install -e '.[tpu]' + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/launch-tpuv7.yml` around lines 36 - 39, The fallback runs pip with system Python after a possibly partially-created virtualenv from `uv sync --extra tpu`; change the logic to check the exit status of `uv sync --extra tpu` and, on failure, remove any partial `.venv`, create and use an explicit virtualenv, and run the fallback install inside that venv (use `python -m venv .venv` then the venv's pip or activate the venv) so `pip install -e '.[tpu]'` installs into the venv rather than the system Python; reference the `uv sync --extra tpu` command and the `pip install -e '.[tpu]'` fallback when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/launch-tpuv7.yml`:
- Around line 33-39: The uv sync step currently creates a project virtualenv but
later verification runs use system Python; update the workflow so dependencies
are executed inside the venv: run uv sync --extra tpu (optionally add --no-dev
to avoid dev deps) and replace the standalone python verification command with
uv run python -c '...' (or ensure uv creates a predictable venv path and
activate it before running verification). Target the uv invocation lines (uv
sync and the subsequent verification python command) and ensure you either add
--no-dev to uv sync or use uv run so the verification uses the virtualenv
created by uv.
---
Nitpick comments:
In `@scripts/launch-tpuv7.yml`:
- Around line 36-39: The fallback runs pip with system Python after a possibly
partially-created virtualenv from `uv sync --extra tpu`; change the logic to
check the exit status of `uv sync --extra tpu` and, on failure, remove any
partial `.venv`, create and use an explicit virtualenv, and run the fallback
install inside that venv (use `python -m venv .venv` then the venv's pip or
activate the venv) so `pip install -e '.[tpu]'` installs into the venv rather
than the system Python; reference the `uv sync --extra tpu` command and the `pip
install -e '.[tpu]'` fallback when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6d62ed4-3859-4157-92ce-8120faca40c4
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/ci/gpu-tests.sky.yamlCLAUDE.mdpyproject.tomlscripts/launch-tpuv7.yml
| # 3. Install uv | ||
| pip install uv | ||
|
|
||
| # git clone https://github.com/primatrix/ant-pretrain.git | ||
| # cd ant-pretrain && git checkout feat/kda-with-kernel | ||
| # 4. Clone and setup project | ||
| git clone git@github.com:primatrix/pallas-kernel.git ~/pallas-kernel | ||
| cd ~/pallas-kernel | ||
| uv sync --extra tpu || pip install -e '.[tpu]' |
There was a problem hiding this comment.
Consider adding --no-dev flag or ensure uv creates the venv in a predictable location.
The uv sync --extra tpu command will create a virtual environment (typically .venv/ in the project directory). However, the subsequent python -c 'import jax; ...' on line 41 runs without activating this venv or using uv run, so it will use the system Python which won't have the dependencies installed.
🐛 Proposed fix to use uv run for the verification command
# 4. Clone and setup project
git clone git@github.com:primatrix/pallas-kernel.git ~/pallas-kernel
cd ~/pallas-kernel
uv sync --extra tpu || pip install -e '.[tpu]'
- python -c 'import jax; print("Total TPU devices (cores):", jax.device_count())'
+ uv run python -c 'import jax; print("Total TPU devices (cores):", jax.device_count())'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/launch-tpuv7.yml` around lines 33 - 39, The uv sync step currently
creates a project virtualenv but later verification runs use system Python;
update the workflow so dependencies are executed inside the venv: run uv sync
--extra tpu (optionally add --no-dev to avoid dev deps) and replace the
standalone python verification command with uv run python -c '...' (or ensure uv
creates a predictable venv path and activate it before running verification).
Target the uv invocation lines (uv sync and the subsequent verification python
command) and ensure you either add --no-dev to uv sync or use uv run so the
verification uses the virtualenv created by uv.
Summary
pytestfromdevextra to base dependencies so CI environments no longer need--extra devtorchvision,flash-linear-attention, and pin-freenumpyuvsettings inpyproject.toml(compile-bytecode,default-groups,python-downloads)Test plan
uv syncinstalls pytest correctlyuv sync --extra gpuworks without--extra dev🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores