Skip to content

Conversation

@kwkr
Copy link
Contributor

@kwkr kwkr commented Apr 16, 2023

addresses #475

Added SimpleSequentialChain implementation based on the one from the python version

This is my first PR here so please excuse me if I messed some conventions up. The functionality is there but due to some differences between Python and TS classes, I wasn't sure how should I proceed with the validation that is included in the Python version.

I also added the some example docs site for this specific class as it's done for the LLM.

Few things to consider:

  1. I added both integration and unit tests for the implementation. I'm not sure if both of them are required because some of them cover the same scenario (i.e. joining two chains together).
  2. The original implementation validates the chains before creating the SimpleSequentialChain object i.e. it checks whether there is a single input and output for each chain. In the case of the TS implementation, the property outputKey is not available on the BaseChain but as a part of some chains (for example LLMChain). Because I used BaseChain as a type of the chains that are passed to the SimpleSequentialChain the way to check for this would be to do some casting and then check if the property outputKey is available on each chain. I would appreciate some input on how I should proceed with this.
  3. The original implementation assumes that both input and output are strings. Should this condition be also preserved when validating the chains?

I would appreciate some hints on this @nfcampos. Thanks in advance for any input!

@vercel
Copy link

vercel bot commented Apr 16, 2023

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

Name Status Preview Updated (UTC)
langchainjs-docs ✅ Ready (Inspect) Visit Preview Apr 17, 2023 0:09am

@nfcampos
Copy link
Collaborator

This is great @kwkr I'm adding outputKeys to BaseChain here #843 once that's in you can rebase and finish the validation logic

Copy link
Collaborator

@nfcampos nfcampos left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@agola11 (not to do this in PR) but thoughts on adding a new callback method called handleChainIntermediateResult could be used in lots of places and might be a good answer for some things we've been discussing?

@nfcampos
Copy link
Collaborator

@kwkr outputKeys has been merged

@kwkr kwkr force-pushed the 475/SimpleSequentialChain branch from 078dbc2 to e10c922 Compare April 17, 2023 11:55
@kwkr kwkr marked this pull request as ready for review April 17, 2023 11:57
@kwkr kwkr requested a review from nfcampos April 17, 2023 11:57
@nfcampos nfcampos added the lgtm PRs that are ready to be merged as-is label Apr 17, 2023
@kwkr kwkr force-pushed the 475/SimpleSequentialChain branch from e10c922 to 146b004 Compare April 17, 2023 12:04
@nfcampos nfcampos merged commit cd24465 into langchain-ai:main Apr 17, 2023
RohitMidha23 pushed a commit to RohitMidha23/langchainjs that referenced this pull request Apr 18, 2023
* add simple sequential chain implementation

* add tests

* add docs

* fix linting errors

* add full input chains validation

---------

Co-authored-by: harry_squater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm PRs that are ready to be merged as-is

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants