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

Runnables: Add .map() method #9445

Merged
merged 7 commits into from
Aug 23, 2023
Merged

Runnables: Add .map() method #9445

merged 7 commits into from
Aug 23, 2023

Conversation

nfcampos
Copy link
Collaborator

No description provided.

@vercel
Copy link

vercel bot commented Aug 18, 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) Visit Preview Aug 23, 2023 6:52pm

@dosubot dosubot bot added the 🤖:improvement Medium size change to existing code to handle new use-cases label Aug 18, 2023
@nfcampos nfcampos marked this pull request as ready for review August 18, 2023 13:37
@@ -208,6 +208,12 @@ def bind(self, **kwargs: Any) -> Runnable[Input, Output]:
"""
return RunnableBinding(bound=self, kwargs=kwargs)

def each(self) -> Runnable[List[Input], List[Output]]:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a better name for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sort of looks like .map to me?

def lc_namespace(self) -> List[str]:
return self.__class__.__module__.split(".")[:-1]

def each(self) -> RunnableEach[Input, Output]: # type: ignore[override]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mypy isn't super smart here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason not to return Runnable[List[Input], List[Output]] wouldn't this preserve the correct interface?

Otherwise, could you leave a comment in the code explaining the need for this type ignore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is Mypy wants this to be Runnable[List[List[Input]], List[List[Output]]]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately I think mypy is correct here:

  • Runnable.map has a signature of -> Runnable[List[Input], List[Output]].
  • RunnableEach[I, O] is a Runnable[List[I], List[O]].
  • That means that in order to subtype Runnable, RunnableEach[I, O].map has to be -> Runnable[List[List[I]], List[List[O]]].

I think this might cause further issues in downstream code, both inside langchain and in downstream packages that depend on it.

It feels like RunnableEach shouldn't be Runnable, and perhaps there's a missing abstraction that should sit underneath both of them?

@nfcampos nfcampos force-pushed the nc/runnables-shared-executor branch from 68535ef to fcb0fbe Compare August 18, 2023 15:06
def lc_namespace(self) -> List[str]:
return self.__class__.__module__.split(".")[:-1]

def each(self) -> RunnableEach[Input, Output]: # type: ignore[override]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason not to return Runnable[List[Input], List[Output]] wouldn't this preserve the correct interface?

Otherwise, could you leave a comment in the code explaining the need for this type ignore?

@nfcampos nfcampos changed the title Runnables: Add .each() method Runnables: Add .map() method Aug 18, 2023
@nfcampos nfcampos force-pushed the nc/runnables-shared-executor branch from fcb0fbe to db4b256 Compare August 23, 2023 18:39
Base automatically changed from nc/runnables-shared-executor to master August 23, 2023 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:improvement Medium size change to existing code to handle new use-cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants