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

Allow callback handlers to opt into being run inline #6424

Merged
merged 2 commits into from Jun 22, 2023

Conversation

nfcampos
Copy link
Collaborator

@nfcampos nfcampos commented Jun 19, 2023

This is useful eg for callback handlers that use context vars (like open telemetry)

See #6095

This is useful eg for callback handlers that use context vars (like open telemetry)
@nfcampos nfcampos requested a review from agola11 June 19, 2023 14:20
@vercel
Copy link

vercel bot commented Jun 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Jun 19, 2023 2:27pm

@@ -259,12 +262,17 @@ async def _ahandle_event(
**kwargs: Any,
) -> None:
"""Generic event handler for AsyncCallbackManager."""
for handler in [h for h in handlers if h.run_inline]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that the same as

_handle_event(sync_handlers)

And then we don't have to handle that case in _ahandle_event_for_handler

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah no because there is other logic in there that I do want to still apply

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the logic exactly the same in these two functions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but then we'd be repeating here the check for whether an event handler is sync or async?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we put the check here and call _handle_event on sync handlers, we don't need to change the _ahandle... function at all.

Sync handlers will never get there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Is that something that should be supported or it's considered as misconfiguration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it won't have any effect, as the concept doesn't make sense. But not sure its worth throwing an exception for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm looking at the current implementation and see that we're not awaiting if run_inline is true. What will happen if we run async function with run_inline and don't await on it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Screenshot 2023-06-19 at 15 39 19

In that branch we already know it's not async, look at the if statement above

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, makes total sense!

Copy link
Contributor

@mariokostelac mariokostelac left a comment

Choose a reason for hiding this comment

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

Thanks a lot for handling this case.

@dev2049 dev2049 merged commit 74ac6fb into master Jun 22, 2023
14 checks passed
@dev2049 dev2049 deleted the nc/callback-handler-run-inline branch June 22, 2023 18:36
This was referenced Jun 25, 2023
kacperlukawski pushed a commit to kacperlukawski/langchain that referenced this pull request Jun 29, 2023
This is useful eg for callback handlers that use context vars (like open
telemetry)

See langchain-ai#6095
aerrober pushed a commit to aerrober/langchain-fork that referenced this pull request Jul 24, 2023
This is useful eg for callback handlers that use context vars (like open
telemetry)

See langchain-ai#6095
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.

None yet

3 participants