-
Notifications
You must be signed in to change notification settings - Fork 2k
Python: Fix sub-workflow checkpoint restore to preserve sub-workflow state #7097
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e786041
a486374
93719f4
1389f30
a4f6c26
e0b0b79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -245,7 +245,13 @@ async def _prepare_checkpoint_state(self) -> None: | |
| self._state.commit() | ||
|
|
||
| async def create_checkpoint_if_enabled(self) -> None: | ||
| """Create a checkpoint if checkpointing is enabled and attach a label and metadata.""" | ||
| """Create a checkpoint and save the checkpoint to the configured storage if one is configured. | ||
|
|
||
| Note: | ||
| 1. This method has no effect if checkpointing is not enabled in the context. | ||
| 2. Do not use this method with ``create_checkpoint_object`` as both methods advance the | ||
| ``previous_checkpoint_id`` and may result in unexpected checkpoint lineage. | ||
| """ | ||
| if not self._ctx.has_checkpointing(): | ||
| return | ||
|
|
||
|
|
@@ -340,6 +346,63 @@ async def restore_from_checkpoint( | |
| logger.error(f"Failed to restore from checkpoint {checkpoint_id}: {e}") | ||
| raise WorkflowCheckpointException(f"Failed to restore from checkpoint {checkpoint_id}") from e | ||
|
|
||
| async def create_checkpoint_object(self) -> WorkflowCheckpoint: | ||
| """Create a checkpoint object. | ||
|
|
||
| Note: | ||
| 1. Do not use this method with ``create_checkpoint_if_enabled`` as both methods advance the | ||
| ``previous_checkpoint_id`` and may result in unexpected checkpoint lineage. | ||
|
|
||
| Returns: | ||
| A ``WorkflowCheckpoint``. | ||
| """ | ||
| # Persist executor snapshots into committed shared state before exporting it. | ||
| await self._prepare_checkpoint_state() | ||
| checkpoint = await self._ctx.create_checkpoint_object( | ||
| self._workflow_name, | ||
| self._graph_signature_hash, | ||
| self._state, | ||
| self._previous_checkpoint_id, | ||
| self._iteration, | ||
| ) | ||
| self._previous_checkpoint_id = checkpoint.checkpoint_id | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we avoid advancing the child lineage until the parent checkpoint is actually persisted? The child snapshot is created before the outer storage save, and outer save failures are intentionally swallowed. In a success/failure/success repro, the last stored parent contained a child checkpoint whose This is a concrete failure mode supporting the concern that the two checkpoint-creation paths need a clearer semantic boundary. |
||
| return checkpoint | ||
|
|
||
| async def restore_from_checkpoint_object(self, checkpoint: WorkflowCheckpoint) -> None: | ||
|
TaoChenOSU marked this conversation as resolved.
|
||
| """Restore runner state from an in-memory ``WorkflowCheckpoint`` object. | ||
|
|
||
| Unlike :meth:`restore_from_checkpoint`, this does not load from a storage | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. after reading this, I still do not understand the difference between this method and |
||
| backend; it applies a checkpoint the caller already holds - for example, a | ||
| child workflow checkpoint embedded in a parent ``WorkflowExecutor``'s state. | ||
|
|
||
| Restores shared state, executor snapshots, in-flight messages, and pending | ||
| request_info events, then marks the runner as resumed. | ||
|
|
||
| Args: | ||
| checkpoint: The checkpoint whose state should be restored. | ||
|
|
||
| Raises: | ||
| WorkflowCheckpointException: If the checkpoint's graph signature does not | ||
| match this runner's workflow, or if restoration otherwise fails. | ||
| """ | ||
| if self._graph_signature_hash != checkpoint.graph_signature_hash: | ||
| raise WorkflowCheckpointException( | ||
| "Workflow graph has changed since the checkpoint was created. " | ||
| "Please rebuild the original workflow before resuming." | ||
| ) | ||
|
|
||
| try: | ||
| # Clear first so import_state (which merges) does not leak stale keys from a | ||
| # prior run on this Workflow instance. | ||
| self._state.clear() | ||
| self._state.import_state(checkpoint.state) | ||
| await self._restore_executor_states() | ||
| await self._ctx.apply_checkpoint(checkpoint) | ||
| self._mark_resumed(checkpoint) | ||
| except Exception as e: | ||
| logger.error(f"Failed to restore from checkpoint {checkpoint.checkpoint_id}: {e}") | ||
| raise WorkflowCheckpointException(f"Failed to restore from checkpoint {checkpoint.checkpoint_id}") from e | ||
|
|
||
| async def _save_executor_states(self) -> None: | ||
| """Populate executor state by calling checkpoint hooks on executors.""" | ||
| for exec_id, executor in self._executors.items(): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -195,6 +195,34 @@ def is_streaming(self) -> bool: | |
| """ | ||
| ... | ||
|
|
||
| async def create_checkpoint_object( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The description has "capture" instead of create, and also should be underscored
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure if I understand. "capture" is used in the comment for the And why does this need to be underscored?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think As Eduard was alluding to, could we name the persistence/ownership contract directly, perhaps |
||
| self, | ||
| workflow_name: str, | ||
| graph_signature_hash: str, | ||
| state: State, | ||
| previous_checkpoint_id: CheckpointID | None, | ||
| iteration_count: int, | ||
| metadata: dict[str, Any] | None = None, | ||
| ) -> WorkflowCheckpoint: | ||
| """Build an in-memory checkpoint of the current context state without persisting it. | ||
|
|
||
| Unlike :meth:`create_checkpoint`, this does not require checkpoint storage and does | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so 1) what happens if checkpoint storage is present and 2) we should find better names, maybe |
||
| not save anything; it returns the checkpoint object for the caller to own. | ||
|
|
||
| Args: | ||
| workflow_name: The name of the workflow for which the checkpoint is being created. | ||
| graph_signature_hash: Hash of the workflow graph topology to | ||
| validate checkpoint compatibility during restore. | ||
| state: The state to include in the checkpoint. | ||
| previous_checkpoint_id: The ID of the previous checkpoint, if any, to form a checkpoint chain. | ||
| iteration_count: The current iteration count of the workflow. | ||
| metadata: Optional metadata to associate with the checkpoint. | ||
|
|
||
| Returns: | ||
| A ``WorkflowCheckpoint`` snapshot of the current context state. | ||
| """ | ||
| ... | ||
|
|
||
| async def create_checkpoint( | ||
| self, | ||
| workflow_name: str, | ||
|
|
@@ -204,7 +232,7 @@ async def create_checkpoint( | |
| iteration_count: int, | ||
| metadata: dict[str, Any] | None = None, | ||
| ) -> CheckpointID: | ||
| """Create a checkpoint of the current workflow state. | ||
| """Create and persist a checkpoint of the current workflow state. | ||
|
|
||
| Args: | ||
| workflow_name: The name of the workflow for which the checkpoint is being created. | ||
|
|
@@ -381,29 +409,48 @@ def clear_runtime_checkpoint_storage(self) -> None: | |
| def has_checkpointing(self) -> bool: | ||
| return self._get_effective_checkpoint_storage() is not None | ||
|
|
||
| async def create_checkpoint( | ||
| async def create_checkpoint_object( | ||
| self, | ||
| workflow_name: str, | ||
| graph_signature_hash: str, | ||
| state: State, | ||
| previous_checkpoint_id: CheckpointID | None, | ||
| iteration_count: int, | ||
| metadata: dict[str, Any] | None = None, | ||
| ) -> CheckpointID: | ||
| storage = self._get_effective_checkpoint_storage() | ||
| if not storage: | ||
| raise ValueError("Checkpoint storage not configured") | ||
|
|
||
| checkpoint = WorkflowCheckpoint( | ||
| ) -> WorkflowCheckpoint: | ||
| return WorkflowCheckpoint( | ||
| workflow_name=workflow_name, | ||
| graph_signature_hash=graph_signature_hash, | ||
| previous_checkpoint_id=previous_checkpoint_id, | ||
| messages=dict(self._messages), | ||
| # Copy the per-source lists so the snapshot is isolated from later context mutations. | ||
| messages={source_id: list(messages) for source_id, messages in self._messages.items()}, | ||
| state=state.export_state(), | ||
| pending_request_info_events=dict(self._pending_request_info_events), | ||
| iteration_count=iteration_count, | ||
| metadata=metadata or {}, | ||
| ) | ||
|
|
||
| async def create_checkpoint( | ||
| self, | ||
| workflow_name: str, | ||
| graph_signature_hash: str, | ||
| state: State, | ||
| previous_checkpoint_id: CheckpointID | None, | ||
| iteration_count: int, | ||
| metadata: dict[str, Any] | None = None, | ||
| ) -> CheckpointID: | ||
| storage = self._get_effective_checkpoint_storage() | ||
| if not storage: | ||
| raise ValueError("Checkpoint storage not configured") | ||
|
|
||
| checkpoint = await self.create_checkpoint_object( | ||
| workflow_name, | ||
| graph_signature_hash, | ||
| state, | ||
| previous_checkpoint_id, | ||
| iteration_count, | ||
| metadata, | ||
| ) | ||
| checkpoint_id = await storage.save(checkpoint) | ||
| logger.debug(f"Created checkpoint {checkpoint_id}") | ||
| return checkpoint_id | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this note to me is a smell that something is wrong, if we have two methods of creating checkpoints, then we need to do a better job of explaining when to use what, especially since they are both public.
Why can't we do
create_checkpointwith embedded logic/params to figure what needs to happen?