-
Notifications
You must be signed in to change notification settings - Fork 15.5k
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
Callbacks Refactor [base] #3256
Conversation
return _configure(cls, inheritable_callbacks, local_callbacks, verbose) | ||
|
||
|
||
T = TypeVar("T", CallbackManager, AsyncCallbackManager) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can think of a better way to do this if it's ugly -- basically want to enforce with typing that typing among arguments and return value must be consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me
docs/ecosystem/gpt4all.md
Outdated
# There are many CallbackHandlers supported, such as | ||
# from langchain.callbacks.streamlit import StreamlitCallbackHandler | ||
|
||
callback_manager = CallbackManager([StreamingStdOutCallbackHandler()]) | ||
model = GPT4All(model="./models/gpt4all-model.bin", n_ctx=512, n_threads=8, callback_handler=callback_handler, verbose=True) | ||
model = GPT4All(model="./models/gpt4all-model.bin", n_ctx=512, n_threads=8, callback_handler=callback_handler, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this line wrong anyway? In the previous line creating a callback_manager, then passing a callback_handler var (that I think doesn't exist) to a callback_handler arg that I also think doesn't exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah, this was messed up by the pycharm refactoring tool
langchain/callbacks/base.py
Outdated
for handler in handlers: | ||
self.add_handler(handler, inherit=inherit) | ||
|
||
def set_handler(self, handler: BaseCallbackHandler, inherit=True) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this method useful enough to exist?
token: str, | ||
run_id: Optional[str] = None, | ||
parent_run_id: Optional[str] = None, | ||
**kwargs: Any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have these kwargs in js, should we?
) | ||
else: | ||
callback_manager = inheritable_callbacks | ||
callback_manager = copy.deepcopy(callback_manager) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why call deep copy directly here rather than the copy method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I ran into this. What's the logic behind a copy here anyway? I'd assume many callback managers—definitely mine—maintain state that cannot be copied such as file handles, network connections, asyncio.Futures etc.
I don't know this code that well, but why even copy at all? It seems somewhat antithetical to the rationale of a callback manager that everything makes a copy of it.
(Not only speaking of the copying of the callback_manager but also all handlers few lines below.)
@@ -75,7 +69,8 @@ def _handle_event( | |||
): | |||
getattr(handler, event_name)(*args, **kwargs) | |||
except Exception as e: | |||
logging.error(f"Error in {event_name} callback: {e}") | |||
# TODO: switch this to use logging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why we have to wait?
Couple additional tools landed today
03e8023
to
a038f37
Compare
def tracing_enabled( | ||
session_name: str = "default", | ||
) -> Generator[TracerSession, None, None]: | ||
"""Get OpenAI callback handler in a context manager.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect docstring?
Modified langchain dependency to prevent error caused by LangChain's recent callbacks refactor ([Callbacks Refactor [base] #3256](langchain-ai/langchain#3256))
Co-authored-by: Nuno Campos <[email protected]> Co-authored-by: Davis Chase <[email protected]> Co-authored-by: Zander Chase <[email protected]> Co-authored-by: Harrison Chase <[email protected]>
@agola11 This was a breaking change. Is it documented somewhere how to migrate? In my case, I did the following:
and replaced it with:
but it broke my app:
Help 😁 |
@pors -- can you send me a snipped of how you were using |
@agola11 The whole project is here: https://github.com/pors/langchain-chat-websockets/blob/main/query_data.py The relevant code is largely based on https://github.com/hwchase17/chat-langchain, so the issue may also be there. |
@pors okay thanks. Looking in a bit |
Once this lands, your issue should be resolved: #3995 -- please comment if that's not the case |
Thanks! It works again for me. I like the fast release schedule :) |
No description provided.