Summary
Observation documents that exactly one of text, messages, or token_ids should be set, but this invariant is currently not validated at construction time.
Consequently:
Observation() fails only later during prompt conversion.- An observation containing multiple representations is silently accepted, with
_obs_to_prompt() selecting one according to its branch order.
Proposed change
Add a lightweight __post_init__() validation to Observation that raises ValueError unless exactly one representation is non-None.
The validation would use is not None, so intentionally empty representations such as Observation(messages=[]) remain valid.
I would also add CPU-only unit tests covering:
- Each valid representation
- Empty strings and empty lists
- No representation
- Conflicting combinations of multiple representations
This change would apply only to Observation. Action would remain unchanged because the rollout loop intentionally constructs it with both decoded text and token IDs.
Would this validation align with the intended API contract? I’d be happy to submit a PR if so.
Summary
Observationdocuments that exactly one oftext,messages, ortoken_idsshould be set, but this invariant is currently not validated at construction time.Consequently:
Observation()fails only later during prompt conversion._obs_to_prompt()selecting one according to its branch order.Proposed change
Add a lightweight
__post_init__()validation toObservationthat raisesValueErrorunless exactly one representation is non-None.The validation would use
is not None, so intentionally empty representations such asObservation(messages=[])remain valid.I would also add CPU-only unit tests covering:
This change would apply only to
Observation.Actionwould remain unchanged because the rollout loop intentionally constructs it with both decoded text and token IDs.Would this validation align with the intended API contract? I’d be happy to submit a PR if so.