Conversation
sangkyoonnam
left a comment
There was a problem hiding this comment.
The method does what #502 asks and the test covers step and reset. One decision I'd like settled: could we keep the ValueError for a __PRIOR_STEP that names an action the graph no longer has, and say so in the docstring?
That case is reachable: a completed checkpoint restored with resume_at_next_action=True keeps the persisted position in __PRIOR_STEP, and initialize_from doesn't check it against the graph, so a rename or removal between runs lands here. With .with_state(**{"__PRIOR_STEP": "removed"}) the two methods disagree: get_next_action() returns None (the adjacency map is a defaultdict, so the lookup finds no transitions), while get_prior_action() raises ValueError from Graph.get_action. I'd rather have the raise, since the state no longer matches the graph; the docstring should then also mention the ValueError for a prior action the graph doesn't have. Mirroring get_next_action() is the other option, as long as it's written down.
On the current/prior wording from #502, measured on this branch: inside a pre_run_step hook it returns the previous action, inside post_run_step the action that finished ([('pre', 'a', None), ('post', 'a', 'a'), ('pre', 'b', 'a'), ('post', 'b', 'b')] with two counter actions). For streaming, __PRIOR_STEP moves when the stream is consumed to completion, not when it starts. A sentence in the docstring would close that part of the issue.
tests/core/test_application.py passes here, 140 tests.
Summary
Application.get_prior_action(), the counterpart ofget_next_action()Noneif nothing has run yet__PRIOR_STEPvalue that is already kept in stateCloses #502
Testing
test_app_get_prior_action, it checks before any step, after each step and afterreset_to_entrypoint()tests/core/test_application.pypasses locally, pre-commit hooks pass