fix(core): Add pass observations for replay training, observe_event API, and fix stale legal_actions - #169
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves replay-based training fidelity and simplifies online inference by extending replay iteration to include implicit “pass” decisions, introducing an event-driven observation API, and fixing stale legal_actions state after start_game in both 4P and 3P state machines.
Changes:
- Add synthetic
ActionType::Passsamples toKyokuStepIterator/KyokuStepIterator3Pfor players who could have claimed a discard but did not. - Add Python API
RiichiEnv.apply_event(event)andRiichiEnv.observe_event(event, player_id)plus integration tests for observe-driven inference flows. - Clear stale action state on
StartGameand adjust round start/turn state soget_observation()doesn’t surface pre-event discard actions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
riichienv-core/src/replay/mod.rs |
Queue and emit synthetic pass observations during replay iteration; add helper(s) to detect immediate claimers. |
riichienv-core/src/state/event_handler.rs |
Track reaction windows by populating current_claims/active_players; clear stale state on StartGame (4P). |
riichienv-core/src/state_3p/event_handler.rs |
Same as 4P plus kita reaction handling; clear stale state on StartGame (3P). |
riichienv-python/src/env.rs |
Expose apply_event and new observe_event convenience API in Python bindings. |
src/riichienv/_riichienv.pyi |
Add stubs/docs for apply_event and observe_event. |
tests/env/test_apply_event.py |
New integration tests validating observe_event behavior in 4P and 3P. |
README.md |
Document the event-driven API and recommended online inference usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Fixed. I'm fairly confident this is correct, but will compare behavior cloning models trained before and after the fix to confirm the issue is resolved. Training will take a few hours. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| let skip_check = matches!( | ||
| ev, | ||
| MjaiEvent::StartGame { .. } | ||
| | MjaiEvent::StartKyoku { .. } | ||
| | MjaiEvent::ReachAccepted { .. } | ||
| | MjaiEvent::Dora { .. } | ||
| | MjaiEvent::Hora { .. } | ||
| | MjaiEvent::Ryukyoku { .. } | ||
| | MjaiEvent::EndKyoku | ||
| | MjaiEvent::EndGame | ||
| | MjaiEvent::Other | ||
| ); | ||
|
|
||
| with_variant_mut!(self, |s| s.apply_mjai_event(ev)); |
There was a problem hiding this comment.
matches!(ev, ...) moves ev (MjaiEvent is not Copy), so ev cannot be passed to apply_mjai_event(ev) afterwards. This should not compile as written. Use matches!(&ev, ...) (or compute skip_check after applying the event) to avoid consuming ev before it is applied.
There was a problem hiding this comment.
This is a false positive. matches!(ev, MjaiEvent::StartGame { .. } | ...) expands to a match expression where every arm uses { .. } wildcard patterns that bind no fields, so no move occurs. ev remains valid for the subsequent apply_mjai_event(ev) call.
|
Verified with a quick smoke test. Ran a 3P game with the pre-fix model (seats 0–1) vs the post-fix model (seat 2). The post-fix model correctly produces Pass actions (4 passes observed), while the pre-fix models produce zero — confirming that pass decisions are now being learned. from riichienv import RiichiEnv, ActionType
from riichienv_ml.agents import Agent
CONFIG_PATH = "src/riichienv_ml/configs/3p/bc_logs.yml"
MODEL_PATH = "/data/workspace/riichienv-ml/3p/bc_logs_6Mr4.pth" # before fix
MODEL2_PATH = "/data/workspace/riichienv-ml/3p/bc_logs_6Mr5_step500000.pth" # after fix
agent = Agent(CONFIG_PATH, MODEL_PATH, device="cuda")
agent2 = Agent(CONFIG_PATH, MODEL2_PATH, device="cuda")
agents = {0: agent, 1: agent, 2: agent2}
env = RiichiEnv(game_mode="3p-red-half")
obs_dict = env.reset()
pass_count = {0: 0, 1: 0, 2: 0}
while not env.done():
actions = {pid: agents[pid].act(obs) for pid, obs in obs_dict.items()}
for pid, action in actions.items():
if action.action_type == ActionType.Pass:
pass_count[pid] += 1
obs_dict = env.step(actions)
print(env.ranks(), env.scores())
print("Pass counts:", pass_count)$ uv run python scripts/test_bc_3p.py
[2, 3, 1] [43100, 12100, 49800]
Pass counts: {0: 0, 1: 0, 2: 4} |
Fix a critical bug in
KyokuStepIteratorwhere pass (not calling) decisions were missing from replay iteration, making it impossible to learn when not to call pon/chi/ron.😇KyokuStepIteratornow yields synthetic observations for players who implicitly declined a claim opportunity (pon/chi/ron) after an opponent's discard. Each observation is paired with anActionType::Passaction, enabling behaviour cloning models to learn when not to call — eliminating the positive-action bias present in replay-only training data.observe_event(event, player_id)API. A new method that applies an MJAI event and returns anObservationonly when the specified player has legal actions. ReturnsNonefor non-decision events (start_game, start_kyoku, dora, hora, ryukyoku, etc.). This simplifies online inference code by combining state update and observation retrieval into a single call. The existingapply_event(event)(renamed fromapply_mjai_event) remains available for fire-and-forget replay parsing.legal_actionsafterstart_game. The constructor's internalreset()leftcurrent_player = 0andphase = WaitAct, soget_observation(0)returned stale discard actions before any real events were applied. Added aStartGamehandler that setscurrent_playerto the sentinel value and clearsactive_players, consistent withStartKyoku. Fixed in both 4P and 3P state machines.