docs: guide for sliding puzzle example - #961
Conversation
|
Hi @slikhite-1 . There is another doc failure. Can you run: to make sure all failures are resolved before we retry? |
|
@slikhite-1 can you rebase since this branch is now in conflict with main |
|
@slikhite-1 can you please rebase so that we can merge this ? |
Signed-off-by: slikhite-1 <slikhite@nvidia.com>
Signed-off-by: slikhite-1 <slikhite@nvidia.com>
Signed-off-by: slikhite-1 <slikhite@nvidia.com>
WalkthroughAdds a new Sliding Puzzle GRPO guide, links it in the docs index, and updates the example GRPO sliding puzzle configuration by reducing sequence length and puzzle shuffle/max move counts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Trainer
participant Policy as LLM Policy
participant Runner as SlidingPuzzleRunner
participant Logic as SlidingPuzzleGameLogic
participant Env as SlidingPuzzleEnv (Ray)
rect rgb(235, 245, 255)
note right of Trainer: Training step (per turn)
Trainer->>Policy: Generate action (XML-wrapped)
Policy-->>Trainer: Action text
Trainer->>Runner: Submit action
end
rect rgb(245, 235, 255)
note over Runner,Logic: Turn processing
Runner->>Runner: Parse & validate format
Runner->>Logic: Validate move
Logic-->>Runner: Valid/Invalid, next state
Runner->>Env: Apply move / fetch state
Env-->>Runner: State snapshot
Runner->>Runner: Compute reward & termination
end
Runner-->>Trainer: Observation, reward, done/terminated
Trainer->>Policy: Update via GRPO (batch/step)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
examples/configs/grpo_sliding_puzzle.yaml (3)
20-33: Avoid setting max_new_tokens equal to total sequence length.With max_new_tokens=${policy.max_total_sequence_length}, prompt+generated tokens can exceed vLLM max_model_len; vLLM will truncate or error. Leave headroom.
Apply one of these:
- max_new_tokens: ${policy.max_total_sequence_length} + # Leave headroom for prompt + max_new_tokens: 768or, if supported:
- max_new_tokens: ${policy.max_total_sequence_length} + max_new_tokens: ${eval:${policy.max_total_sequence_length} - 256}
56-59: Even shuffle_moves conflicts with doc logic; may exceed configured max.Guide’s generator makes shuffle_moves odd by adding 1 when even; with a max of 10, a sampled 10 becomes 11 (exceeding the configured “maximum”). Either set an odd max here or adjust the generation rule.
Apply this diff (config-side fix):
- shuffle_moves: 10 # Number of random moves to shuffle the solved state + shuffle_moves: 11 # Odd to avoid post-adjustment exceeding maxAlternative (doc/code-side): sample only odd values ≤ max, or decrement when even.
1-76: Add trailing newline to satisfy yamllint.File lacks a newline at EOF.
docs/guides/grpo-sliding-puzzle.md (7)
16-20: “Default 2×2 configuration” contradicts the shipped config (size: 5).Either change the wording or pass an explicit override in the example.
Apply one:
- Text: “Train a model with the provided config (default max size 5; generator samples sizes 2..size).”
- Or command:
-uv run python examples/run_grpo_sliding_puzzle.py +uv run python examples/run_grpo_sliding_puzzle.py env.sliding_puzzle_game.cfg.game_config.size=2
26-30: Even shuffle_moves (10) vs. “odd-only” generation rule.Your generator forces odd by +1 when even; using 10 in docs can become 11. Pick an odd value or call this out.
Example:
- env.sliding_puzzle_game.cfg.game_config.shuffle_moves=10 + env.sliding_puzzle_game.cfg.game_config.shuffle_moves=11Or update the prose: “If an even number is provided, the generator uses the next odd value.”
37-42: Inconsistent uv invocation.Elsewhere you use “uv run python …”; here it’s missing “python”.
Apply:
-uv run examples/run_grpo_sliding_puzzle.py \ +uv run python examples/run_grpo_sliding_puzzle.py \
55-79: Add a language for the fenced block (markdownlint MD040).Use “text” to satisfy linters.
Apply:
-``` +```text …--- `141-144`: **Label the fenced block (markdownlint MD040).** This isn’t code; “text” is fine. ```diff -``` +```text Training Size = num_prompts_per_step × num_generations_per_prompt × max_num_steps Validation Size = max_val_samples--- `282-287`: **Label the YAML snippet (markdownlint MD040).** ```diff -``` +```yaml game_config: size: 5 # Size of the puzzle (e.g., 2 for 2x2, 3 for 3x3) shuffle_moves: 10 # Number of random moves to shuffle the solved state max_moves: 30--- `121-130`: **Doc generation rule can exceed configured maximum.** The “+1 if even” can push beyond max (e.g., 10 → 11). Consider sampling from odd numbers ≤ max or decrementing when even. Proposed snippet: ```python max_shuffle = max_config.get("shuffle_moves") shuffle_moves = random.randrange(1 if max_shuffle % 2 == 1 else 2, max_shuffle + 1, 2)Or:
shuffle_moves = random.randint(1, max_shuffle) shuffle_moves -= (shuffle_moves % 2 == 0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/assets/train-reward-sliding-puzzle.pngis excluded by!**/*.pngdocs/assets/valid_acc-sliding-puzzle.pngis excluded by!**/*.png
📒 Files selected for processing (3)
docs/guides/grpo-sliding-puzzle.md(1 hunks)docs/index.md(1 hunks)examples/configs/grpo_sliding_puzzle.yaml(3 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
examples/configs/grpo_sliding_puzzle.yaml
[error] 76-76: no new line character at the end of file
(new-line-at-end-of-file)
🪛 markdownlint-cli2 (0.17.2)
docs/guides/grpo-sliding-puzzle.md
55-55: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
141-141: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
282-282: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Post submodule check comment / Comment on PR
- GitHub Check: Post automodel integration comment / Comment on PR
🔇 Additional comments (3)
examples/configs/grpo_sliding_puzzle.yaml (1)
21-47: Model/context tuning looks sane.Reducing policy.max_total_sequence_length to 1024 and tying vLLM max_model_len accordingly is consistent with the updated example and smaller model. No issues spotted.
Please confirm typical prompt length so the new max_new_tokens leaves enough margin.
docs/index.md (1)
31-31: Guide added to toctree — looks good.Link path and ordering are consistent with the Guides section.
docs/guides/grpo-sliding-puzzle.md (1)
291-295: Images verified — asset paths point to existing files
Found docs/assets/train-reward-sliding-puzzle.png and docs/assets/valid_acc-sliding-puzzle.png; ../assets/... from docs/guides/grpo-sliding-puzzle.md resolves correctly, no change needed.
|
@terrykong please review and help merge if all ok |
|
@terrykong I guess it's still asking for your review in spite of auto merge. |
Pull Request is not mergeable
Signed-off-by: slikhite-1 <slikhite@nvidia.com>
Signed-off-by: slikhite-1 <slikhite@nvidia.com> Signed-off-by: yuanhangs <yuanhangs@nvidia.com>
What does this PR do ?
Added docs and reward curve for sliding puzzle multi-turn tool calling example
Summary by CodeRabbit