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: Protect Langchain callbacks #1653

Merged
merged 9 commits into from
Oct 25, 2023

Conversation

anticorrelator
Copy link
Contributor

resolves #1615

Shields Langchain instrumentation errors from causing exceptions in user code.

src/phoenix/trace/langchain/tracer.py Outdated Show resolved Hide resolved
tests/trace/langchain/test_tracer.py Outdated Show resolved Hide resolved
Comment on lines 268 to 277
def _null_fallback(
serialized: Dict[str, Any],
messages: List[List[BaseMessage]],
*,
run_id: UUID,
tags: Optional[List[str]] = None,
parent_run_id: Optional[UUID] = None,
metadata: Optional[Dict[str, Any]] = None,
**kwargs: Any,
) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

why not make its intent even more clear?

Suggested change
def _null_fallback(
serialized: Dict[str, Any],
messages: List[List[BaseMessage]],
*,
run_id: UUID,
tags: Optional[List[str]] = None,
parent_run_id: Optional[UUID] = None,
metadata: Optional[Dict[str, Any]] = None,
**kwargs: Any,
) -> None:
def _null_fallback(*_: Any, **__: Any) -> 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.

I don't think of this as a general purpose fallback, just a alternate code path this one specific method. E.g. it doesn't have to actually be a no-op, is this a strong opinion you hold regarding this call signature?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a matter of opinion, but, as you have pointed out, there are two intentions that are subtly different:

  1. It doesn't have to be no-op (so it's open to adding more code)
  2. It should be no-op (no, don't add anything here)

I understand that 1 applies in the general case, but in this specific instance, 2 is what I think we want, and if that's the case then what I was asking for is to dial up the degree of specificity in what we are communicating here.

Alternatively, If 1 is actually what we want, i.e., letting users develop a separate code path, then we should leave a comment letting developers know that any errors here will not be caught.

Lastly, we can also keep the verbose signature and leave a comment that says "don't add anything" but that is uneconomical because it leaves us with two separate locations for maintaining the call signature (just to keep them in sync) even though one of the locations is always a no-op.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally don't believe verbosity or repeated signatures are too onerous to maintain in the sense that I think they should be clearly and directly linked (maybe I should even pick a better name for the fallback function). The function as it looks now is what I intend for this to look and feel like (providing an explicit alternate codepath that has exactly the same usable signature), though I understand if you feel differently.

I'm mostly interested in what you think the shortest resolution might be

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine, I guess I reached with the opposite conclusion from reading the code alone, so a few clarifying comments will resolve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, let me give that a try and if it still feels bad to you I'll move to the generalized signature

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it would also have to be renamed to something other than null_fallback

@anticorrelator anticorrelator merged commit 196c22b into main Oct 25, 2023
9 checks passed
@anticorrelator anticorrelator deleted the dustin/protect-langchain-callbacks branch October 25, 2023 03:19
mikeldking pushed a commit that referenced this pull request Oct 30, 2023
* Add graceful fallback to langchain tracer

* Address feedback

* Rename fallback and add informative comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[ENHANCEMENT] protect langchain callbacks from unhandled errors
3 participants