Skip to content

refactor: accept sync callbacks in run_async - #11466

Merged
anakin87 merged 5 commits into
v3from
relax-select-streaming-callback
Jun 3, 2026
Merged

refactor: accept sync callbacks in run_async#11466
anakin87 merged 5 commits into
v3from
relax-select-streaming-callback

Conversation

@anakin87

@anakin87 anakin87 commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Related Issues

Proposed Changes:

  • allow sync callbacks to be used in run_async (with a warning)
  • slightly adapt Chat Generators in Haystack

How did you test it?

CI, new tests

Checklist

  • I have read the contributors guidelines and the code of conduct.
  • I have updated the related issue with new insights and changes.
  • I have added unit tests and updated the docstrings.
  • I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test: and added ! in case the PR includes breaking changes.
  • I have documented my code.
  • I have added a release note file, following the contributors guidelines.
  • I have run pre-commit hooks and fixed any issue.

@vercel

vercel Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
haystack-docs Ignored Ignored Preview Jun 1, 2026 4:51pm

Request Review

anakin87
anakin87 commented Jun 1, 2026
View reviewed changes
Comment thread haystack/utils/asynchronous.py
@@ -1,18 +0,0 @@
# SPDX-FileCopyrightText: 2022-present deepset GmbH <info@deepset.ai>

@anakin87 anakin87 Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this was only used internally. I moved it to streaming_chunk.py

anakin87
anakin87 commented Jun 1, 2026
View reviewed changes
Comment thread test/dataclasses/test_streaming_chunk.py
with pytest.warns(Warning, match="dataclasses.replace"):
chunk.content = "other"


@anakin87 anakin87 Jun 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

New tests starting here

@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)		Lines missing
  haystack/components/agents
  tool_calling.py
  haystack/components/generators/chat
  hugging_face_api.py
  openai.py
  openai_responses.py 553-562
  haystack/core/pipeline
  async_pipeline.py
  haystack/dataclasses
  streaming_chunk.py
  haystack/utils
  hf.py
Project Total  

This report was generated by python-coverage-comment-action

@anakin87
anakin87 marked this pull request as ready for review June 1, 2026 17:05
@anakin87
anakin87 requested a review from a team as a code owner June 1, 2026 17:05
@anakin87
anakin87 requested review from bogdankostic and sjrl and removed request for a team and bogdankostic June 1, 2026 17:05
@anakin87

anakin87 commented Jun 1, 2026

Copy link
Copy Markdown
Member Author

@mpangrazzi feel free to take a look if you want

@anakin87 anakin87 changed the title refactor: allow sync callbacks in run_async refactor: accept sync callbacks in run_async Jun 1, 2026
sjrl
sjrl reviewed Jun 2, 2026
View reviewed changes
Comment thread haystack/components/generators/chat/openai_responses.py
Comment on lines 432 to 434
streaming_callback = select_streaming_callback(
init_callback=self.streaming_callback, runtime_callback=streaming_callback, requires_async=True
)

@sjrl sjrl Jun 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't we also need to drop this async enforcement?

@sjrl sjrl Jun 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ahh I see you updated the method so requires_async=True only emits a warning now. I think fair enough, are we not changing the name of the param since the method is public?

It's probably also fine to leave but it does feel a bit odd to say requires_async=True but that now means we only emit a warning. But since it's mostly an internal method it's probably fine to leave.

@anakin87 anakin87 Jun 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I agree with you: I'd prefer to rename the parameter to async_context, as require_async is a bit misleading now.

Unfortunately, this would be a breaking change and I'd like to avoid forcing integrations to bump their Haystack version to 3. Plus, I don't know if this Haystack method is used somewhere else in the ecosystem.
So I'd leave the name unchanged for now. Happy to re-discuss this in the future.

sjrl
sjrl approved these changes Jun 2, 2026
View reviewed changes

@sjrl sjrl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a minor comment about naming, otherwise looks good!

@anakin87
anakin87 merged commit 2b09dbb into v3 Jun 3, 2026
33 of 35 checks passed
@anakin87
anakin87 deleted the relax-select-streaming-callback branch June 3, 2026 08:56
@julian-risch julian-risch mentioned this pull request Jul 5, 2026
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sjrl sjrl sjrl approved these changes

Assignees

No one assigned

Labels

topic:core topic:tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@anakin87 @sjrl