Add installation instructions for pip and Docker to README.md#2854
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe README.md file was updated to expand the Installation section by adding detailed instructions for installing via pip and Docker. These additions provide explicit commands for both methods, clarifying the installation process for users. No existing content was modified or removed. Changes
Suggested labels
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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
CodeRabbit Configuration File (
|
![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 (1)
README.md (1)
62-70: Re-consider strict version pinning and missing prerequisite stepsHard-pinning
packaging==23.2,setuptools==75.8.0, etc. may downgrade a user’s existing environment or break other packages.
Additionally, most users need to upgradepipfirst (the--no-build-isolationflag depends on reasonably newpip).-pip3 install -U packaging==23.2 setuptools==75.8.0 wheel ninja +python3 -m pip install -U pip +# Only pin if there is a demonstrated incompatibility; otherwise leave unpinned +pip3 install -U packaging setuptools wheel ninjaConsider adding a brief note explaining why these exact versions are required, or relax the pins if they are merely recommendations.
Also, torch isn’t installed here; users will hit a runtime error immediately afterwards. Either mention that torch must be installed separately (with the proper CUDA build) or include it in the command.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: preview
| #### Using Docker | ||
|
|
||
| ```bash | ||
| docker run --gpus '"all"' --rm -it axolotlai/axolotl:main-latest | ||
| ``` |
There was a problem hiding this comment.
docker run --gpus '"all"' is likely to fail – use the canonical syntax
The extra quoting passes the literal string "all" (with quotes) to Docker on most shells, resulting in:
docker: Error response from daemon: unknown capability `"all"`.
Replace with the standard flag:
-docker run --gpus '"all"' --rm -it axolotlai/axolotl:main-latest
+docker run --gpus all --rm -it axolotlai/axolotl:main-latestOptionally add a note that this requires the NVIDIA Container Toolkit.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #### Using Docker | |
| ```bash | |
| docker run --gpus '"all"' --rm -it axolotlai/axolotl:main-latest | |
| ``` | |
| #### Using Docker | |
🤖 Prompt for AI Agents
In README.md around lines 73 to 77, the Docker run command uses incorrect
quoting for the --gpus flag, causing it to pass the literal string with quotes
and fail. Replace the flag --gpus '"all"' with the standard syntax --gpus all
without extra quotes. Additionally, add a note mentioning that this requires the
NVIDIA Container Toolkit to be installed for GPU support.
There was a problem hiding this comment.
I'm thinking whether we should add --ipc=host as a default to our docker commands too (and also in installation doc) as the user will likely hit shared memory failure too.
There was a problem hiding this comment.
🧩 Analysis chain
@NanoCode012 That's an excellent point! The --ipc=host flag is indeed crucial for ML workloads to prevent shared memory issues with PyTorch DataLoaders and multiprocessing.
Based on the search results, I can see that Axolotl likely uses multiprocessing heavily during training, and users would definitely encounter shared memory limitations with the default Docker IPC settings.
The suggested Docker command should be:
This would prevent the common "cannot allocate memory in static TLS block" and similar shared memory errors that users typically encounter when running PyTorch training in Docker containers.
You're right that this should be consistent across both the README and the installation documentation. Would you like me to check what the current installation docs contain for Docker commands and suggest a unified approach?
🏁 Script executed:
#!/bin/bash
# Search for existing Docker commands and documentation
echo "=== Searching for Docker commands in documentation ==="
rg "docker run" -A 2 -B 2
echo -e "\n=== Searching for shared memory or IPC mentions ==="
rg -i "(shared.?memory|ipc|--ipc)" -A 2 -B 2
echo -e "\n=== Looking for installation documentation ==="
fd -t f -e md | grep -i install | head -5
echo -e "\n=== Checking for Docker-related documentation files ==="
fd -t f -e md | xargs grep -l -i docker | head -5
Length of output: 2431
🏁 Script executed:
docker run --gpus all --ipc=host --rm -it axolotlai/axolotl:main-latest
Length of output: 170
@NanoCode012 Perfect observation! The search results reveal a significant inconsistency in the documentation that supports your suggestion completely.
Current State:
- README.md (basic):
docker run --gpus '"all"' --rm -it axolotlai/axolotl:main-latest❌ (no shared memory config) - docs/installation.qmd (basic): Same simple command ❌
- docs/installation.qmd (advanced):
docker run --privileged --gpus '"all"' --shm-size 10g --rm -it --name axolotl --ipc=host ...✅ - docs/debugging.qmd: Similar advanced command with
--ipc=host✅
The Problem:
Users following the README or basic installation instructions will hit shared memory failures, while only those who dig into the advanced configuration sections will find the working commands.
Recommended Approach:
For the README, I'd suggest updating the basic Docker command to:
docker run --gpus all --ipc=host --rm -it axolotlai/axolotl:main-latestQuestion for you: Should we also include --shm-size 10g in the basic command? The advanced configs use it, but it might make the "quick start" command look more intimidating. The --ipc=host alone should handle most shared memory issues, but --shm-size provides additional headroom.
This standardization would prevent users from hitting the common PyTorch multiprocessing failures right from their first attempt with Axolotl.
✏️ Learnings added
Learnt from: NanoCode012
PR: axolotl-ai-cloud/axolotl#2854
File: README.md:73-77
Timestamp: 2025-07-02T02:56:20.752Z
Learning: For Axolotl Docker commands, the `--ipc=host` flag should be included by default to prevent shared memory failures that commonly occur with PyTorch DataLoaders and multiprocessing during machine learning training workflows.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Isn't this covered in the installation docs https://github.com/axolotl-ai-cloud/axolotl/blob/main/docs/installation.qmd#L89-L112 -> https://docs.axolotl.ai/docs/installation.html#sec-docker? Maybe linking it in a higher visibility way would be more appropriate? |
I think both would be beneficial. The reasoning is, that most users only read the docs when the have issues. If we can fix those issues earlier by pointing them to use docker instead, then would have less issues from wrong installations. |
djsaunde
left a comment
djsaunde
left a comment
There was a problem hiding this comment.
Maybe just add a quick one-liner re: how docker can be less error-prone.
4 participants
Description
Motivation and Context
How has this been tested?
Screenshots (if appropriate)
Types of changes
Social Handles (Optional)
Summary by CodeRabbit