Doc/readme update - #32
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR rebrands project documentation as SAfactory, replaces generic examples with Geo3K workflows, adds environment and runtime references, and documents storage, RL, RJob, Sandbox, and agent-skill workflows. ChangesSAfactory entrypoints and Geo3K onboarding
Environment and runtime contracts
Storage and trajectory data reference
RL and execution modes
Agent workflow skill
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/guides/evaluation.md`:
- Around line 22-39: Update the Geo3K command examples to use the local
smoke-test config path instead of the unavailable default: replace the
hard-coded agent-config path in docs/guides/evaluation.md lines 22-39 and
docs/guides/evaluation_CN.md lines 22-39, and use the copied config path in
docs/reference/configuration.md lines 16-42 and
docs/reference/configuration_CN.md lines 16-42.
In `@docs/guides/S3`+LanceDB-storage.md:
- Line 33: Remove the stray trailing “x” from the final sentence in the
S3/LanceDB storage documentation so it ends with “documentation.”
- Line 8: Clean up the installation command in the S3+LanceDB storage guide by
removing the injected review text after the requirements file argument. Leave
the command as only the valid pip install invocation shown in the review.
- Line 19: Update the WT_SDK_S3_ALLOW_HTTP example value to false in
docs/guides/S3+LanceDB-storage.md at line 19 and
docs/guides/S3+LanceDB-storage_CN.md at line 19. Document that true is an
explicit override only for non-TLS test endpoints.
In `@docs/reference/gateway.md`:
- Line 91: Make the launcher command’s --llm-model route key match the
configured dsv4pro route in docs/reference/gateway.md:91-91 and
docs/reference/gateway_CN.md:91-91, or explicitly reference the corresponding
geo3k_model configuration in both documents. Apply the same correction
consistently in both Gateway examples.
In `@gateway/config.example.yaml`:
- Around line 48-50: Update the example llm_routes key from YOUR_ROUTE_KEY to
geo3k_model so it matches the model value used by the Geo3K quickstart commands
and allows route lookup to succeed when the configuration is copied unchanged.
🪄 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 Plus
Run ID: ebf1679d-7778-460e-9f12-e6f200fbd213
⛔ Files ignored due to path filters (5)
fig/agentic_sandbox.PNGis excluded by!**/*.pngfig/ai_sandbox_v1.jpegis excluded by!**/*.jpegfig/relationship.pngis excluded by!**/*.pngfig/scope_capability_safety_aaai_trend.pngis excluded by!**/*.pngfig/visualize.gifis excluded by!**/*.gif
📒 Files selected for processing (26)
.gitignoreREADME.mdREADME_CN.mddocs/environments.mddocs/environments_CN.mddocs/guides/S3+LanceDB-storage.mddocs/guides/S3+LanceDB-storage_CN.mddocs/guides/custom-environment.mddocs/guides/custom-environment_CN.mddocs/guides/data-manager.mddocs/guides/data-manager_CN.mddocs/guides/evaluation.mddocs/guides/evaluation_CN.mddocs/guides/rl-training.mddocs/guides/rl-training_CN.mddocs/internal/rjob-mode.mddocs/internal/rjob-mode_CN.mddocs/internal/sandbox-mode.mddocs/internal/sandbox-mode_CN.mddocs/reference/configuration.mddocs/reference/configuration_CN.mddocs/reference/environments.mddocs/reference/environments_CN.mddocs/reference/gateway.mddocs/reference/gateway_CN.mdgateway/config.example.yaml
💤 Files with no reviewable changes (2)
- docs/environments_CN.md
- docs/environments.md
| Geo3K example: | ||
|
|
||
| ```bash | ||
| python launcher.py \ | ||
| --agent-config env/mybench/mybench_config.yaml \ | ||
| --agent-start-config env/mybench/mybench_start.yaml \ | ||
| --mode docker \ | ||
| --agent-config env/geo3k/geo3k_config.yaml \ | ||
| --agent-start-config env/geo3k/geo3k_start.yaml \ | ||
| --gateway-base-url http://127.0.0.1:8000/v1/sessions \ | ||
| --llm-model YOUR_ROUTE_KEY \ | ||
| --llm-model geo3k_model \ | ||
| --enable-evaluation \ | ||
| --db-path sqlite://env_trajs.db \ | ||
| --pool-size 1 | ||
| --job-id geo3k-eval-smoke \ | ||
| --pool-size 1 \ | ||
| --max-workers 1 \ | ||
| --max-steps 10 | ||
| ``` | ||
|
|
||
| For a first local run, use a Geo3K smoke-test config that points to `env/geo3k/datasets/geo3k_sample.jsonl` if the default Geo3K config points to a full parquet dataset that is not available locally. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wire the local smoke-test config into every command. The fallback notes create or recommend a local config, but the commands continue passing the unavailable default config.
docs/guides/evaluation.md#L22-L39: replace the hard-codedenv/geo3k/geo3k_config.yamlwith the local smoke-test config path.docs/guides/evaluation_CN.md#L22-L39: replace the hard-codedenv/geo3k/geo3k_config.yamlwith the local smoke-test config path.docs/reference/configuration.md#L16-L42: show the copied config path in--agent-config.docs/reference/configuration_CN.md#L16-L42: show the copied config path in--agent-config.
📍 Affects 4 files
docs/guides/evaluation.md#L22-L39(this comment)docs/guides/evaluation_CN.md#L22-L39docs/reference/configuration.md#L16-L42docs/reference/configuration_CN.md#L16-L42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/guides/evaluation.md` around lines 22 - 39, Update the Geo3K command
examples to use the local smoke-test config path instead of the unavailable
default: replace the hard-coded agent-config path in docs/guides/evaluation.md
lines 22-39 and docs/guides/evaluation_CN.md lines 22-39, and use the copied
config path in docs/reference/configuration.md lines 16-42 and
docs/reference/configuration_CN.md lines 16-42.
| Install the optional dependencies: | ||
|
|
||
| ```bash | ||
| pip install -r requirements-cloud.txtExpand commentComment on line R203Resolved |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the install command before publishing.
The command contains injected review text and will fail when copied. It should be only pip install -r requirements-cloud.txt.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/guides/S3`+LanceDB-storage.md at line 8, Clean up the installation
command in the S3+LanceDB storage guide by removing the injected review text
after the requirements file argument. Leave the command as only the valid pip
install invocation shown in the review.
| WT_SDK_DB_URI=s3://YOUR_DATA_DATABASE | ||
| WT_SDK_ENV_CONFIG_DB_URI=s3://YOUR_ENV_CONFIG_DATABASE | ||
| WT_SDK_S3_ENDPOINT=https://YOUR_S3_ENDPOINT | ||
| WT_SDK_S3_ALLOW_HTTP=true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not default S3 to plaintext HTTP
Both docs set WT_SDK_S3_ALLOW_HTTP=true, which makes the main example prefer insecure HTTP. Default this to false and treat true as an explicit override for non-TLS test endpoints.
📍 Affects 2 files
docs/guides/S3+LanceDB-storage.md#L19-L19(this comment)docs/guides/S3+LanceDB-storage_CN.md#L19-L19
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/guides/S3`+LanceDB-storage.md at line 19, Update the
WT_SDK_S3_ALLOW_HTTP example value to false in docs/guides/S3+LanceDB-storage.md
at line 19 and docs/guides/S3+LanceDB-storage_CN.md at line 19. Document that
true is an explicit override only for non-TLS test endpoints.
| set +a | ||
| ``` | ||
|
|
||
| Then set the gateway `storage_type` to `cloud` and launch Safactory with `--storage-type cloud`. The `production` profile selects the production landing/serving tables, while `test` selects the test tables. See [AI45Lab/wt-data-platform-sdk](https://github.com/AI45Lab/wt-data-platform-sdk) for the complete configuration and table documentation.x No newline at end of file |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the trailing x.
The final sentence currently ends with documentation.x.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/guides/S3`+LanceDB-storage.md at line 33, Remove the stray trailing “x”
from the final sentence in the S3/LanceDB storage documentation so it ends with
“documentation.”
| YOUR_ROUTE_KEY: | ||
| base_url: http://YOUR_LLM_HOST/v1 | ||
| api_key: YOUR_API_KEY |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the example route key with the Geo3K quickstarts.
llm_routes is keyed by the value passed to --llm-model, but this example defines only YOUR_ROUTE_KEY while the new Geo3K commands use geo3k_model. Copying this config and following those commands fails route lookup. Either define geo3k_model here or explicitly instruct users to replace the CLI value with their configured key.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gateway/config.example.yaml` around lines 48 - 50, Update the example
llm_routes key from YOUR_ROUTE_KEY to geo3k_model so it matches the model value
used by the Geo3K quickstart commands and allows route lookup to succeed when
the configuration is copied unchanged.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/guides/rl-training.md`:
- Line 154: Update the rollout startup paragraph to make its behavior
conditional on AIEVOBOX_MODE, explicitly documenting the docker, rjob, and
sandbox differences or clearly labeling the existing Docker rollout instructions
as Docker-only. Ensure users are not told that Docker rollout collection always
starts when another mode is configured.
- Around line 183-185: Update the AIEVOBOX_DB_URL documentation in the
environment-variable table to describe both SQLite and cloud-storage URL/backend
formats. Explicitly document the required matching storage type and backend/URL
values across Gateway, launcher, and Buffer Server, while preserving the
existing SQLite guidance.
- Around line 111-115: Update the environment-variable setup around SLIME_HOME,
MEGATRON_HOME, HF_CKPT_DIR, and SAVE_DIR so it does not supply nonexistent
/path/to/... defaults. Make these runtime paths explicitly required or clearly
mark each placeholder as a mandatory value to replace before running training;
preserve LOAD_DIR’s fallback to HF_CKPT_DIR.
🪄 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 Plus
Run ID: 607bdb29-14f1-46ed-bf11-eb9f3f6a434d
📒 Files selected for processing (4)
docs/guides/evaluation.mddocs/guides/evaluation_CN.mddocs/guides/rl-training.mddocs/guides/rl-training_CN.md
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/guides/rl-training_CN.md
- docs/guides/evaluation.md
- docs/guides/evaluation_CN.md
| export SLIME_HOME="${SLIME_HOME:-/path/to/slime}" | ||
| export MEGATRON_HOME="${MEGATRON_HOME:-/path/to/Megatron-LM}" | ||
| export HF_CKPT_DIR="${HF_CKPT_DIR:-/path/to/hf-checkpoint}" | ||
| export LOAD_DIR="${LOAD_DIR:-${HF_CKPT_DIR}}" | ||
| export SAVE_DIR="${SAVE_DIR:-/path/to/save/checkpoints}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not make required runtime paths default to nonexistent placeholders.
Following this “Minimal configuration” verbatim leaves SLIME_HOME, MEGATRON_HOME, HF_CKPT_DIR, and SAVE_DIR pointing at /path/to/...; the training commands will then fail or load/save from invalid locations. Make these values required, or clearly mark them as mandatory edits before startup.
Proposed adjustment
-export SLIME_HOME="${SLIME_HOME:-/path/to/slime}"
-export MEGATRON_HOME="${MEGATRON_HOME:-/path/to/Megatron-LM}"
-export HF_CKPT_DIR="${HF_CKPT_DIR:-/path/to/hf-checkpoint}"
+export SLIME_HOME="${SLIME_HOME:?Set SLIME_HOME to the Slime checkout}"
+export MEGATRON_HOME="${MEGATRON_HOME:?Set MEGATRON_HOME to the Megatron-LM checkout}"
+export HF_CKPT_DIR="${HF_CKPT_DIR:?Set HF_CKPT_DIR to the model checkpoint}"
export LOAD_DIR="${LOAD_DIR:-${HF_CKPT_DIR}}"
-export SAVE_DIR="${SAVE_DIR:-/path/to/save/checkpoints}"
+export SAVE_DIR="${SAVE_DIR:?Set SAVE_DIR to a writable checkpoint directory}"📝 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.
| export SLIME_HOME="${SLIME_HOME:-/path/to/slime}" | |
| export MEGATRON_HOME="${MEGATRON_HOME:-/path/to/Megatron-LM}" | |
| export HF_CKPT_DIR="${HF_CKPT_DIR:-/path/to/hf-checkpoint}" | |
| export LOAD_DIR="${LOAD_DIR:-${HF_CKPT_DIR}}" | |
| export SAVE_DIR="${SAVE_DIR:-/path/to/save/checkpoints}" | |
| export SLIME_HOME="${SLIME_HOME:?Set SLIME_HOME to the Slime checkout}" | |
| export MEGATRON_HOME="${MEGATRON_HOME:?Set MEGATRON_HOME to the Megatron-LM checkout}" | |
| export HF_CKPT_DIR="${HF_CKPT_DIR:?Set HF_CKPT_DIR to the model checkpoint}" | |
| export LOAD_DIR="${LOAD_DIR:-${HF_CKPT_DIR}}" | |
| export SAVE_DIR="${SAVE_DIR:?Set SAVE_DIR to a writable checkpoint directory}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/guides/rl-training.md` around lines 111 - 115, Update the
environment-variable setup around SLIME_HOME, MEGATRON_HOME, HF_CKPT_DIR, and
SAVE_DIR so it does not supply nonexistent /path/to/... defaults. Make these
runtime paths explicitly required or clearly mark each placeholder as a
mandatory value to replace before running training; preserve LOAD_DIR’s fallback
to HF_CKPT_DIR.
| bash rl/run_buffer_server.sh --env rl/examples/my_env/env.sh | ||
| ``` | ||
|
|
||
| Buffer Server auto-starts Gateway by default, generates `logs/gateway.rl.generated.yaml`, routes `RL_MODEL` to the Slime-hosted LLM proxy, starts Docker rollout collection, and serves completed groups through `/get_rollout_data`. Stop any manually started Gateway on the same port before using autostart. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make rollout startup wording conditional on AIEVOBOX_MODE.
AIEVOBOX_MODE supports docker, rjob, and sandbox, but this sentence says Buffer Server always starts Docker rollout collection. Document the configured mode explicitly, or mark this paragraph as Docker-only; otherwise RJob and Sandbox users receive incorrect startup expectations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/guides/rl-training.md` at line 154, Update the rollout startup paragraph
to make its behavior conditional on AIEVOBOX_MODE, explicitly documenting the
docker, rjob, and sandbox differences or clearly labeling the existing Docker
rollout instructions as Docker-only. Ensure users are not told that Docker
rollout collection always starts when another mode is configured.
| | `AIEVOBOX_MODE` | `--mode` | `docker`, `rjob`, or `sandbox`. | | ||
| | `STORAGE_TYPE` | `--storage-type` | `sqlite` or `cloud`. | | ||
| | `AIEVOBOX_DB_URL` | `--db-path` | SQLite URI used by launcher, Gateway, and Buffer Server. | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document the cloud-storage contract for AIEVOBOX_DB_URL.
STORAGE_TYPE is documented as supporting both sqlite and cloud, while AIEVOBOX_DB_URL is described only as a SQLite URI. Add the required cloud URL/backend format and explain which values must match between Gateway, launcher, and Buffer Server; otherwise cloud deployments can be configured with an invalid or mismatched storage contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/guides/rl-training.md` around lines 183 - 185, Update the
AIEVOBOX_DB_URL documentation in the environment-variable table to describe both
SQLite and cloud-storage URL/backend formats. Explicitly document the required
matching storage type and backend/URL values across Gateway, launcher, and
Buffer Server, while preserving the existing SQLite guidance.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@env/geo3k/geo3k_config.yaml`:
- Line 6: Update the dataset setting in geo3k_config.yaml to reference a parquet
file path that load_yaml_configs() and load_dataset_file() can resolve under the
configured base directory, rather than the unresolved Hugging Face identifier;
alternatively, ensure a verified download/materialization step runs before
configuration loading.
🪄 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 Plus
Run ID: 1320bbca-ad3d-44cd-a19e-003c3f77c9f9
📒 Files selected for processing (1)
env/geo3k/geo3k_config.yaml
d4de663 to
aedbf69
Compare
Summary by CodeRabbit
.gitignorecoverage for private*.local.yamlfiles.