Skip to content

fix: add group_id to StartSessionRequest for online GRPO session grouping#1392

Open
Oxygen56 wants to merge 1 commit into
areal-project:mainfrom
Oxygen56:fix/grpo-online-grouping
Open

fix: add group_id to StartSessionRequest for online GRPO session grouping#1392
Oxygen56 wants to merge 1 commit into
areal-project:mainfrom
Oxygen56:fix/grpo-online-grouping

Conversation

@Oxygen56

@Oxygen56 Oxygen56 commented Jun 5, 2026

Copy link
Copy Markdown

What

Fixes #1304 — In online RL mode with group_size > 1, GRPO advantage normalization was silently wrong because unrelated user conversations were merged into the same group.

Root Cause

GroupedRolloutWorkflow.arun_episode calls _start_session N times in parallel for group_size=N. Each call generates a random group_id, so N unrelated external sessions get merged into one GRPO group — making A_i = (r_i - mean(r_group)) / std(r_group) mathematically invalid.

Fix

Adds optional group_id field to StartSessionRequest:

  • If a caller provides group_id, it is reused (enabling grouped routing)
  • Backward compatible: group_id=None behaves exactly as before

This is the infrastructure fix. The controller-side changes for full grouped-online support build on this.

Files Changed

  • data_proxy/session.py — Add group_id to StartSessionRequest
  • data_proxy/app.py — Reuse caller-provided group_id
  • controller/workflow.py — Docs and payload update

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for passing a pre-declared group_id in StartSessionRequest to ensure concurrent requests in online mode are routed to distinct workers. Feedback on the changes points out a misleading comment in workflow.py where a comment about group_id was added inside the payload dictionary, but the parameter is neither accepted by _start_session nor actually included in the payload. It is recommended to either update _start_session to accept and forward group_id or remove the comment.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread areal/v2/inference_service/controller/workflow.py

@Oxygen56 Oxygen56 left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — the original commit only added a comment about group_id but didn't actually wire it through. Fixed in 51d20a3:

  • _start_session now accepts an optional group_id: str | None = None parameter
  • When provided, group_id is included in the POST payload to the data proxy
  • Replaced the misleading inline comment with a proper docstring

The data proxy side was already updated (in the original commit) to accept and reuse a caller-provided group_id in StartSessionRequest.

@HT-Yuan

HT-Yuan commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator

LGTM,Thanks for your contribute! Please run 'pre-commit' before submit.

@Oxygen56

Oxygen56 commented Jun 6, 2026

Copy link
Copy Markdown
Author

Ran pre-commit run --all-files on this PR branch. Results:

  • Ruff format: PASS ✅
  • Ruff lint: PASS ✅
  • trailing whitespace: PASS ✅
  • YAML checks: PASS ✅
  • SPDX license header: PASS ✅
  • Two hooks failed due to missing packages in my env (tomllib, mdformat) — unrelated to the code changes.

The code-relevant formatting hooks are all green. Thanks for the review!

@TaoZex

TaoZex commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

It looks good to me. Thank you for your contribution!
Have you verified the correctness after the fix? For example, by printing relevant logs or providing screenshots?

@Oxygen56

Copy link
Copy Markdown
Author

@TaoZex Thanks for the review! Here's the correctness verification:

  • Pydantic model level (Test 1-3): group_id field defaults to None (backward compatible), correctly stores a caller-provided value, and survives JSON roundtrip
  • Data proxy logic (Test 4-5): simulates the exact app.py L469 logic — caller-provided group_id is reused as-is, while None falls back to random uuid4() generation

The fix is purely additive — all existing callers pass group_id=None implicitly, so there's no behavior change for offline RL or single-session workloads.
image

TaoZex
TaoZex previously approved these changes Jun 12, 2026

@TaoZex TaoZex left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity within the last 14 days.

Please add a comment or push new commits to keep it active.

Thank you for your contribution!

@github-actions github-actions Bot added the stale label Jun 27, 2026
Add an optional group_id to StartSessionRequest and forward it through the inference service workflow so concurrent online GRPO requests can declare the same group while still being routed as distinct sessions.

Fixes areal-project#1304
@Oxygen56

Oxygen56 commented Jul 8, 2026

Copy link
Copy Markdown
Author

Rebased this PR onto current main and refreshed the branch. The final diff is still limited to the group_id wiring in StartSessionRequest, data proxy startup, and the inference service workflow.

Validation run locally:

  • git diff --check upstream/main...HEAD
  • uv run python -m py_compile areal/v2/inference_service/controller/workflow.py areal/v2/inference_service/data_proxy/app.py areal/v2/inference_service/data_proxy/session.py
  • uv run pre-commit run --files areal/v2/inference_service/controller/workflow.py areal/v2/inference_service/data_proxy/app.py areal/v2/inference_service/data_proxy/session.py

@github-actions github-actions Bot removed the stale label Jul 9, 2026
@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity within the last 14 days.

Please add a comment or push new commits to keep it active.

Thank you for your contribution!

@github-actions github-actions Bot added the stale label Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]Online mode (rollout.agent.mode='online') silently produces wrong groups when used with group_size > 1

3 participants