Skip to content

[DBA-137] Add the stop button#30

Merged
23jura23 merged 7 commits intomainfrom
ykhudyakov/add-stop-button
Mar 12, 2026
Merged

[DBA-137] Add the stop button#30
23jura23 merged 7 commits intomainfrom
ykhudyakov/add-stop-button

Conversation

@23jura23
Copy link
Contributor

@23jura23 23jura23 commented Mar 5, 2026

Still a bit buggy, requires more testing

@23jura23 23jura23 marked this pull request as ready for review March 6, 2026 21:01
@23jura23 23jura23 requested review from a team and gasparian March 6, 2026 21:01
Copy link
Contributor

@gasparian gasparian left a comment

Choose a reason for hiding this comment

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

I've added unittests for the query executor, feel free to revert back. It's just easier to understand what could go wrong here. Will try to generate e2e test, just cuious would it be possible to catch anything there)

@gasparian
Copy link
Contributor

gasparian commented Mar 9, 2026

I've added unittests for the query executor, feel free to revert back. It's just easier to understand what could go wrong here. Will try to generate e2e test, just cuious would it be possible to catch anything there)

So I've decided to add a separate race condition tests as regular tests. Ptal if it makes sense. For now - everything pass.

@catstrike
Copy link
Collaborator

I don't think we should merge this in the current form.

The current approach terminates the thread directly, which is problematic because it can happen at an unexpected time.

Cancellation should be expressed at the API level instead. A better design would be for Thread.ask() to return a handle that exposes a cancel() method. The caller could then request cancellation without directly interfering with the thread itself.

The Executor already contains a loop that can handle cancellation gracefully:

for mode, chunk in compiled_graph.stream(
    start_state,
    stream_mode=["values", "messages"],
    config=config,
    **kwargs,
):

With a proper cancellation signal, this loop could exit cleanly without forcefully terminating the thread.

Copy link
Collaborator

@SokolovYaroslav SokolovYaroslav left a comment

Choose a reason for hiding this comment

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

Lenar's concern is valid, however we'll probably move from streamlit, so it's not that critical

…utton

Made-with: Cursor

# Conflicts:
#	src/databao_cli/ui/components/chat.py
@23jura23 23jura23 merged commit 1e27007 into main Mar 12, 2026
5 checks passed
@23jura23 23jura23 deleted the ykhudyakov/add-stop-button branch March 12, 2026 21:57
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.

4 participants