Skip to content

Conversation

@alecf
Copy link
Contributor

@alecf alecf commented Jul 11, 2023

This lets you pass callbacks when you create the summarize chain:

summarize = load_summarize_chain(llm, chain_type="map_reduce", callbacks=[my_callbacks])
summary = summarize(documents)

See #5572 for a similar surgical fix.

tagging @hwchase17 for callbacks work

@vercel
Copy link

vercel bot commented Jul 11, 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) Jul 24, 2023 9:11pm

Copy link
Collaborator

Choose a reason for hiding this comment

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

this introduces the converse problem, of making it hard to create a map reduce docs chain with only callbacks on the MapReduceDocumentsChain.

maybe we call this arg something like shared_callbacks? and for the final map reduce chain we check if kwargs contains 'callbacks', and only used shared_callbacks if it doesn't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is that kind of flexibility thing that people want? I think this is an area where I'm a little confused on best practices in langchain.

What I feel like I'm seeing in most of the codebase is that callbacks are passed into agent/chain/llm constructors or utility functions, and that means "all children of this object get the same callbacks" are passed down to child chains / llms / etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be worth noting that the docs call out this general pattern here:

The callbacks argument is available on most objects throughout the API (Chains, Models, Tools, Agents, etc.) in two different places:

  • Constructor callbacks: defined in the constructor, eg. LLMChain(callbacks=[handler], tags=['a-tag']), which will be used for all calls made on that object, and will be scoped to that object only, eg. if you pass a handler to the LLMChain constructor, it will not be used by the Model attached to that chain.
  • Request callbacks: defined in the run()/apply() methods used for issuing a request, eg. chain.run(input, callbacks=[handler]), which will be used for that specific request only, and all sub-requests that it contains (eg. a call to an LLMChain triggers a call to a Model, which uses the same handler passed in the call() method).

The verbose argument is available on most objects throughout the API (Chains, Models, Tools, Agents, etc.) as a constructor argument, eg. LLMChain(verbose=True), and it is equivalent to passing a ConsoleCallbackHandler to the callbacks argument of that object and all child objects. This is useful for debugging, as it will log all events to the console.

i.e. the notion of "shared" callbacks is the default rather than the exception.

This particular root function (load_summarize_chain) is technically not a constructor but it is doing a tremendous amount of scaffolding and calling constructors that would otherwise take callbacks.

The way this case seems to be addressed elsewhere in the codebase is to add a xxx_kwargs parameter, e.g. agent_kwargs. So here you could have doc_chain_kwargs but personally that feels like an awkward way to change this interface to support what would seem to me to be a somewhat esoteric case (supporting only callbacks on MapReduceDocumentsChain)

@alecf
Copy link
Contributor Author

alecf commented Jul 13, 2023

@d1sounds - this is an example of some of the callbacks propagation I've been working on

@alecf alecf force-pushed the alecf/summarize-with-callbacks branch from 64540f0 to 046b5f3 Compare July 24, 2023 21:10
@alecf alecf requested a review from baskaryan July 24, 2023 21:11
hwchase17 pushed a commit that referenced this pull request Aug 4, 2023
…uffDocumentsChain (#7853)

This is another case, similar to #5572 and #7565 where the callbacks are
getting dropped during construction of the chains.

tagging @hwchase17 and @agola11 for callbacks propagation

<!-- Thank you for contributing to LangChain!

Replace this comment with:
  - Description: a description of the change, 
  - Issue: the issue # it fixes (if applicable),
  - Dependencies: any dependencies required for this change,
- Tag maintainer: for a quicker response, tag the relevant maintainer
(see below),
- Twitter handle: we announce bigger features on Twitter. If your PR
gets announced and you'd like a mention, we'll gladly shout you out!

If you're adding a new integration, please include:
1. a test for the integration, preferably unit tests that do not rely on
network access,
  2. an example notebook showing its use.

Maintainer responsibilities:
  - General / Misc / if you don't know who to tag: @baskaryan
  - DataLoaders / VectorStores / Retrievers: @rlancemartin, @eyurtsev
  - Models / Prompts: @hwchase17, @baskaryan
  - Memory: @hwchase17
  - Agents / Tools / Toolkits: @hinthornw
  - Tracing / Callbacks: @agola11
  - Async: @agola11

If no one reviews your PR within a few days, feel free to @-mention the
same people again.

See contribution guidelines for more information on how to write/run
tests, lint, etc:
https://github.com/hwchase17/langchain/blob/master/.github/CONTRIBUTING.md
 -->
Copy link
Contributor

@hwchase17 hwchase17 left a comment

Choose a reason for hiding this comment

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

thanks!

@hwchase17 hwchase17 merged commit 5d76540 into langchain-ai:master Aug 4, 2023
@alecf alecf deleted the alecf/summarize-with-callbacks branch August 8, 2023 16:04
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.

3 participants