Skip to content
This repository has been archived by the owner on Nov 13, 2024. It is now read-only.

Add last message query generator #210

Merged

Conversation

izellevy
Copy link
Collaborator

@izellevy izellevy commented Dec 10, 2023

Problem

Some LLMs do not support function calls. We need a basic query generator for this kind of APIs.

Solution

Add a super basic query generator that takes the last message and returns the message as is as a query.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Infrastructure change (CI configs, etc)
  • Non-code change (docs, etc)
  • None of the above: (explain here)

Test Plan

Added necessary unit tests.

Copy link
Contributor

@igiloh-pinecone igiloh-pinecone left a comment

Choose a reason for hiding this comment

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

@izellevy see one comment

if len(messages) == 0:
raise ValueError("Passed chat history does not contain any messages."
" Please include at least one message in the history.")
return [Query(text=messages[-1].content)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please take the last message which is a UserMessage (message.role == 'Role.user' or isinstance()).
If you go backwards on the messages and can't find a UserMessage - then raise an error

Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds a bit weird to me. It's more likely that if the last message is not user message, there is something wrong in the pipeline or usage of this class, so IMO it's better to raise an error here instead of silently do something that might not be expected by the user

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see your point @igiloh-pinecone but agree with @acatav. Maybe I can raise if the history is empty or the last message is not a user message @igiloh-pinecone wdyt?

Copy link
Contributor

@acatav acatav left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@igiloh-pinecone igiloh-pinecone left a comment

Choose a reason for hiding this comment

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

LGTM

Since Anyscale don't support function calling for now
@igiloh-pinecone igiloh-pinecone added this pull request to the merge queue Dec 10, 2023
@igiloh-pinecone igiloh-pinecone removed this pull request from the merge queue due to a manual request Dec 10, 2023
@igiloh-pinecone igiloh-pinecone added this pull request to the merge queue Dec 10, 2023
Merged via the queue into pinecone-io:main with commit 405d73d Dec 10, 2023
10 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants