Conversation
Summary of ChangesHello @garrett4wade, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the AReaL project's documentation by introducing detailed guides and references for critical features. It covers checkpointing for both model export and fault recovery, a unified metrics tracking system for distributed training, and a comprehensive explanation of GPU allocation modes. Furthermore, it provides an in-depth reference for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request significantly improves the documentation by adding comprehensive guides on checkpointing, metrics tracking, allocation modes, and agent workflows, and reorganizing existing tutorials. It also includes a cleanup by removing the unused AREAL_RECOVER_RUN environment variable. However, a security audit identified critical vulnerabilities, including multiple instances of Command Injection in the local and Slurm launchers, and an Insecure Deserialization vulnerability in the recovery handler. These issues stem from the use of unsanitized data from external sources in sensitive sinks such as subprocess.Popen(shell=True), bash script templates, and pickle.load(). Additionally, minor suggestions have been made to fix typos and improve formatting for consistency and clarity in the new documentation. It is strongly recommended to sanitize all external inputs and avoid using insecure execution patterns to mitigate these security risks.
I am having trouble creating individual review comments. Click here to see my feedback.
areal/launcher/local.py (370)
The LocalLauncher.submit_array method (lines 141-148) constructs a shell command string by concatenating environment variables and the command itself, and then executes it using subprocess.Popen with shell=True. Since some environment variables (like AREAL_LLM_SERVER_ADDRS on line 369) are sourced from an external name resolution service (line 349), an attacker who can manipulate the name resolution service can achieve arbitrary code execution on the local machine. Although this PR removes one environment variable from the dictionary, the underlying command injection vulnerability remains in the modified block.
areal/launcher/slurm.py (612)
The SlurmLauncher constructs an sbatch script by inserting environment variables and command strings directly into a bash template. This script is then written to a file and executed. Values sourced from the name resolution service (e.g., llm_addrs on line 553) are joined into AREAL_LLM_SERVER_ADDRS and inserted into the script without sanitization. An attacker manipulating the name resolution service can inject malicious bash commands into the sbatch script, leading to Remote Code Execution (RCE) on the Slurm cluster. This PR modifies the environment variable dictionary used in this process, but does not address the lack of sanitization.
areal/scheduler/slurm.py (404)
Environment variables prepared in _prepare_worker_specs are later used to construct sbatch scripts in SlurmScheduler._generate_sbatch_script. These variables are not sanitized before being inserted into the bash script template. An attacker who can control the environment or the configuration can inject arbitrary commands into the sbatch script. This PR modifies the environment variable preparation logic but leaves the injection risk unaddressed.
areal/utils/recover.py (228-229)
The RecoverInfo.load method uses pickle.load() to deserialize dataloader state from a file on disk (line 116). The path to this file is constructed from the experiment and trial names, typically located on a shared filesystem. If an attacker can write to this filesystem, they can replace the pickle file with a malicious one, leading to arbitrary code execution when the training run is recovered. By removing the check for the AREAL_RECOVER_RUN environment variable (lines 228-229), this PR expands the attack surface by allowing the vulnerable load method to be called more easily during initialization.
docs/best_practices/workflow.md (17)
This code snippet uses AsyncOpenAI, workflow_context, and aiofiles without importing them. To make the example clearer and more self-contained for users, it's good practice to include all necessary imports.
from openai import AsyncOpenAI
from areal.infra import workflow_context
import aiofiles
# Or the `run` method in agent workflows
docs/reference/alloc_mode.md (79)
There appears to be a typo in "backed". It should likely be "backend".
Note that the internal backend configurations do not affect how AReaL allocate GPUs.
docs/reference/checkpointing.md (192-194)
The numbered list items for the recovery process are all marked with 1.. While many Markdown renderers will automatically create a sequential list (1, 2, 3), it's better practice to number them sequentially in the source for clarity and to ensure consistent rendering across all platforms.
1. `RecoverHandler.load()` restores all saved state (if any)
2. Training continues from `last_step_info.next().global_step`
3. Inference engine weights are synchronized to match recovered state
docs/reference/metrics_tracking.md (34)
Using bold text for subheadings (**Characteristics:**) is less conventional than using markdown heading syntax (e.g., ### Characteristics). Using proper headings improves the document's structure, makes it easier to navigate, and allows for direct linking to sections. This applies to other similar subheadings in this file (lines 41, 65, 94, 101, 129).
### Characteristics
docs/reference/rollout_workflow.md (154)
The code example for implementing a custom workflow uses uuid and ModelRequest without importing them. This could lead to confusion or copy-paste errors for users. It's a good practice to include all necessary imports in documentation examples to make them self-contained and runnable.
from areal.api.workflow_api import RolloutWorkflow
from areal.api.io_struct import ModelRequest
import uuid
docs/tutorial/gsm8k_grpo.md (274)
There's a typo in the word "perferrable". It should be "preferable".
`agenerate` API. It is preferable if you want more fine-grained control over token IDs.
Add comprehensive reference documentation for core AReaL systems: - Checkpointing: Saver (HF format) and RecoverHandler (DCP) docs - Metrics tracking: Streaming vs batch paradigms, distributed aggregation - RolloutWorkflow: Interface, return types, and implementation patterns - Allocation mode: Resource allocation strategies reference Additional changes: - Refactor recover.py: Simplify mode handling (deprecate fault/resume in favor of on/off/auto/disabled), add mode validation with migration hints - Add unit tests for recovery configuration validation - Reorganize tutorials: Move troubleshooting content to reference docs, expand eval guide, clarify gsm8k_grpo architecture walkthrough - Add workflow best practices guide
…-project#891) * docs: add checkpointing, metrics tracking, and workflow guides Add comprehensive reference documentation for core AReaL systems: - Checkpointing: Saver (HF format) and RecoverHandler (DCP) docs - Metrics tracking: Streaming vs batch paradigms, distributed aggregation - RolloutWorkflow: Interface, return types, and implementation patterns - Allocation mode: Resource allocation strategies reference Additional changes: - Refactor recover.py: Simplify mode handling (deprecate fault/resume in favor of on/off/auto/disabled), add mode validation with migration hints - Add unit tests for recovery configuration validation - Reorganize tutorials: Move troubleshooting content to reference docs, expand eval guide, clarify gsm8k_grpo architecture walkthrough - Add workflow best practices guide * fix doc typo * replace dash with hyphen
…-project#891) * docs: add checkpointing, metrics tracking, and workflow guides Add comprehensive reference documentation for core AReaL systems: - Checkpointing: Saver (HF format) and RecoverHandler (DCP) docs - Metrics tracking: Streaming vs batch paradigms, distributed aggregation - RolloutWorkflow: Interface, return types, and implementation patterns - Allocation mode: Resource allocation strategies reference Additional changes: - Refactor recover.py: Simplify mode handling (deprecate fault/resume in favor of on/off/auto/disabled), add mode validation with migration hints - Add unit tests for recovery configuration validation - Reorganize tutorials: Move troubleshooting content to reference docs, expand eval guide, clarify gsm8k_grpo architecture walkthrough - Add workflow best practices guide * fix doc typo * replace dash with hyphen
Description
Add comprehensive reference documentation for core AReaL systems including checkpointing,
metrics tracking, rollout workflows, and allocation modes. Also includes recovery system
refactoring with simplified mode handling and unit tests.
Related Issue
N/A
Type of Change
Checklist
jb build docs/gemini review)Breaking Change Details (if applicable):
N/A
Additional Context
Key changes:
New Documentation:
docs/reference/checkpointing.md: Saver (HF format) and RecoverHandler (DCP) docsdocs/reference/metrics_tracking.md: Streaming vs batch paradigms, distributed aggregationdocs/reference/rollout_workflow.md: Interface, return types, implementation patternsdocs/reference/alloc_mode.md: Resource allocation strategies referencedocs/best_practices/workflow.md: Workflow implementation best practicesCode Changes:
areal/utils/recover.py: Simplified mode handling (deprecate fault/resume in favor of on/off/auto/disabled)areal/api/cli_args.py: Add mode validation with migration hintsareal/tests/test_recover.py: Unit tests for recovery configurationTutorial Reorganization:
docs/tutorial/troubleshooting.md(content moved to reference docs)docs/tutorial/eval.mdwith more detailsdocs/tutorial/gsm8k_grpo.mdarchitecture walkthrough