Skip to content
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

[bug fix] Ensure that StopEvent gets cleared from Context._in_progress["_done"] after a Workflow run #17300

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

nerdai
Copy link
Contributor

@nerdai nerdai commented Dec 17, 2024

Description

With the addition of Checkpointing feature, we added the Context._in_progress field that marks which steps/events are in progress at the time of the snapshot. (The purpose being that if you reload from a Checkpoint that had some steps/events in progress, then these would have to re-started from scratch when re-running the Workflow.)

In doing so, I introduced a bug that didn't clear (_done step, StopEvent) from the _in_progress attribute when a Workflow has successfully ran. This PR fixes this.

Fixes issues that were observed in llama-deploy:

Type of Change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Your pull-request will likely not be merged unless it is covered by some form of impactful unit testing.

  • I added new unit tests to cover this change and adapted an existing one with a new assertion

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Dec 17, 2024
Copy link
Member

@masci masci left a comment

Choose a reason for hiding this comment

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

Discussed this offline, LGTM!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Dec 17, 2024
@logan-markewich logan-markewich enabled auto-merge (squash) December 17, 2024 18:38
@nerdai nerdai disabled auto-merge December 17, 2024 20:22
@nerdai nerdai merged commit d6b0f70 into main Dec 17, 2024
9 of 11 checks passed
@nerdai nerdai deleted the nerdai/fix-stopevent-not-removed-from-inprogress branch December 17, 2024 20:22
@nerdai
Copy link
Contributor Author

nerdai commented Dec 17, 2024

merging despite anthropic tests not passing -- this should be resolved in #17299

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants