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

feat(assistant): stop a generation #28810

Merged
merged 24 commits into from
Feb 20, 2025
Merged

feat(assistant): stop a generation #28810

merged 24 commits into from
Feb 20, 2025

Conversation

skoob13
Copy link
Contributor

@skoob13 skoob13 commented Feb 17, 2025

Problem

Customers want to stop the generation if things go south.

Changes

  • Add status to the Conversation model that controls if the double-texting is allowed (parallel executions of the same conversation).
  • Exit the node if the conversation was cancelled before we call a run method.
  • Reset the state after an abruption or exception.

Demo

2025-02-17 20 14 04

Recovering after a failed generation

(it wasn't possible before)

Screenshot 2025-02-17 at 17 58 25

Does this work well for both Cloud and self-hosted?

No

How did you test this code?

Manual tests, unit tests.

Copy link
Contributor

github-actions bot commented Feb 17, 2025

Size Change: +4.97 kB (+0.05%)

Total Size: 9.71 MB

Filename Size Change
frontend/dist/toolbar.js 9.71 MB +4.97 kB (+0.05%)

compressed-size-action

@skoob13 skoob13 requested a review from Twixes February 17, 2025 20:04
@skoob13 skoob13 marked this pull request as ready for review February 17, 2025 20:04
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: app.greptile.com/review/github.

18 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@skoob13 skoob13 marked this pull request as draft February 18, 2025 09:46
@skoob13 skoob13 marked this pull request as ready for review February 18, 2025 10:13
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: app.greptile.com/review/github.

21 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Works nicely! Some assorted comments before getting it in

Comment on lines +85 to +95
return cls(
intermediate_steps=[],
plan="",
graph_status="",
memory_updated=False,
memory_collection_messages=[],
root_tool_call_id="",
root_tool_insight_plan="",
root_tool_insight_type="",
root_tool_calls_count=0,
)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be just cls()? I'm just unsure why e.g. the default intermediate_steps value on the model itself isn't list(), rather than None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a workaround for now. LangGraph doesn't correctly merge states, so all None values are ignored despite being explicitly set. More context. intermediate_steps can't be an empty list by default because it will just replace state unexpectedly. I'm planning to migrate pydantic states to a plain TypedDict.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that issue. Alright, let's keep these "manual defaults" then

@skoob13 skoob13 requested a review from Twixes February 19, 2025 18:04
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

⏹️

@Twixes Twixes merged commit a250542 into master Feb 20, 2025
98 checks passed
@Twixes Twixes deleted the feat/max-stop-btn branch February 20, 2025 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants