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

core: Add aformat_messages to FewShotChatMessagePromptTemplate and ChatPromptTemplate #19648

Merged

Conversation

cbornet
Copy link
Collaborator

@cbornet cbornet commented Mar 27, 2024

Needed since the example selector may use a vector store.

Copy link

vercel bot commented Mar 27, 2024

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 Mar 28, 2024 10:51am

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. 🤖:improvement Medium size change to existing code to handle new use-cases labels Mar 27, 2024
@@ -901,19 +929,31 @@ def from_messages(
partial_variables=partial_vars,
)

def format(self, **kwargs: Any) -> str:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed format from ChatPromptTemplate as it is identical to the one in the parent class BaseChatPromptTemplate

@cbornet cbornet force-pushed the async-FewShotChatMessagePromptTemplate branch 2 times, most recently from 1073750 to 8ae8e21 Compare March 27, 2024 11:48
@cbornet cbornet force-pushed the async-FewShotChatMessagePromptTemplate branch from 8ae8e21 to c9ee89f Compare March 27, 2024 11:59
@eyurtsev eyurtsev self-assigned this Mar 27, 2024
@eyurtsev eyurtsev self-requested a review March 27, 2024 20:03
Returns:
List of BaseMessages.
"""
return await run_in_executor(None, self.format_messages, **kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is another place where I feel like we need to delegate to the sync implementation.

I'm a bit worried about having this dichotomy / how to make it obvious that this is is what's going on

Copy link
Collaborator

Choose a reason for hiding this comment

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

cc @baskaryan / @nfcampos

Do we keep run_in_executor for consistency or use a blocking call to the sync code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the part that makes it all weird:

message = message_template.format_messages(**kwargs)

Chat prompt templates are constructed of multiple messages each of which can be formatted independently -- this
would mean that interpolating a few variables into essentially a list of dicts we're launching several threads

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another possibility if we don't want to choose now is to raise NotImplementedError in the default interface.

Copy link
Collaborator Author

@cbornet cbornet Mar 27, 2024

Choose a reason for hiding this comment

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

I intend to implement all subclasses in the end so the default interface won't be called anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

a list of dicts we're launching several threads

Actually, it's not launching several threads, it's submitting tasks to the default thread-pool (which has 1 thread per core).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Anyway, I updated the PR to call the sync method directly. PTAL

@eyurtsev eyurtsev self-requested a review March 27, 2024 20:48
@dosubot dosubot bot added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label Mar 29, 2024
@eyurtsev eyurtsev merged commit 6b2b511 into langchain-ai:master Mar 29, 2024
95 checks passed
@cbornet cbornet deleted the async-FewShotChatMessagePromptTemplate branch March 29, 2024 15:17
gkorland pushed a commit to FalkorDB/langchain that referenced this pull request Mar 30, 2024
… and ChatPromptTemplate (langchain-ai#19648)

Needed since the example selector may use a vector store.
hinthornw pushed a commit that referenced this pull request Apr 26, 2024
… and ChatPromptTemplate (#19648)

Needed since the example selector may use a vector store.
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 lgtm PR looks good. Use to confirm that a PR is ready for merging. size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants