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

Add sources to completions APIs and UI #1206

Merged
merged 4 commits into from
Nov 11, 2023
Merged

Conversation

imartinez
Copy link
Collaborator

@imartinez imartinez commented Nov 11, 2023

Reuse Chunk object to append the sources used during completions to the completions response.
Affects both chat/completions and plain /completions

It has been integrated in the API and UI (see attachments).

Note 1: Chunks returned as sources will always have their previous_texts and next_texts set to none, given those are intended to be only used in the /chunks API - therefore we could eventually separate both objects (Chunks used for source representation, and /chunks API response)

Note 2: this information is also being added to the stream API, making the streaming chunks too big in my opinion, potentially affecting performance; should we remove sources from the streaming API?

Screenshot 2023-11-11 at 10 52 51 Screenshot 2023-11-11 at 10 53 28

@imartinez
Copy link
Collaborator Author

On note 2, we could maybe add an input parameter to the call, same as "stream" that is "include_context_sources" or something like that.

lopagela
lopagela previously approved these changes Nov 11, 2023
Copy link
Contributor

@lopagela lopagela left a comment

Choose a reason for hiding this comment

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

LGTM even though I'm not capable at 100% to validate what is happening 😅

)

@classmethod
def from_node(cls: type["Chunk"], node: NodeWithScore) -> "Chunk":
Copy link
Collaborator

Choose a reason for hiding this comment

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

you couldn't use the Chunk type here? circular reference?

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 reason is that this class method is defined within Chunk class definition. So Chunk type is not ready.

@@ -48,8 +52,11 @@ class OpenAICompletion(BaseModel):
choices: list[OpenAIChoice]

@classmethod
def from_text(
cls, text: str | None, finish_reason: str | None = None
def from_text_and_sources(
Copy link
Collaborator

@pabloogc pabloogc Nov 11, 2023

Choose a reason for hiding this comment

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

tiny nit, but if you name the function from_text_and_sources I would expect the sources to be mandatory, and from_text to still be there but with the sources as an optional arg.

from_text(all_options_nullable), from_text_and_sources(options, non-null sources) -> call from_text

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

pabloogc
pabloogc previously approved these changes Nov 11, 2023
@imartinez imartinez dismissed stale reviews from pabloogc and lopagela via f5fd6ff November 11, 2023 20:10
@imartinez imartinez merged commit a22969a into main Nov 11, 2023
6 checks passed
@lopagela lopagela deleted the feature/sources-in-completions branch November 11, 2023 22:19
simonbermudez pushed a commit to simonbermudez/saimon that referenced this pull request Feb 24, 2024
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