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

feat(traces): add reranking span kind for document reranking in llama index #1588

Merged
merged 13 commits into from
Oct 12, 2023

Conversation

RogerHYang
Copy link
Contributor

resolves #1153

Screen.Recording.2023-10-06.at.5.00.57.PM.mov

Copy link
Contributor

@axiomofjoy axiomofjoy left a comment

Choose a reason for hiding this comment

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

Python code lgtm. Any reason to keep the test you added as an integration test, or can it be made into a unit test?

Let's get a review from @mikeldking for the frontend.

app/src/pages/trace/TracePage.tsx Outdated Show resolved Hide resolved
app/src/pages/trace/TracePage.tsx Outdated Show resolved Hide resolved
@axiomofjoy
Copy link
Contributor

Looks awesome @RogerHYang !

app/schema.graphql Outdated Show resolved Hide resolved
@mikeldking
Copy link
Contributor

we need a followup for langchain too

@mikeldking
Copy link
Contributor

Screenshot 2023-10-10 at 5 29 56 PM The LLM reranker of Llama_index doesn't get the same attributes as the CohereReranker - I think if we are going to calculate things like NCDG we will have to at least capture the attributes from both. Do you know what the root cause of his is?

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Contributor

@mikeldking mikeldking left a comment

Choose a reason for hiding this comment

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

LGTM! I filed some follow-up tickets. Might do a quick audit on the UI post any changes but looks good!

<CodeBlock value={query} mimeType="text" />
</Card>
<Card
title={`Input Documents (${numInputDocuments})`}
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a titleExtra prop on card where you can place a Counter component. https://5f9739a76e154c00220dd4b9-zeknbennzf.chromatic.com/?path=/story/counter--gallery

Comment on lines 648 to 649
title={`Re-ranked Documents (${numOutputDocuments})`}
{...defaultCardProps}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above - rely on titleExtra and counter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it and it looks like this.

Screenshot 2023-10-12 at 8 59 14 AM

app/src/pages/trace/TracePage.tsx Outdated Show resolved Hide resolved
{input_documents.map((document, idx) => {
return (
<li key={idx}>
<DocumentItem document={document} />
Copy link
Contributor

Choose a reason for hiding this comment

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

I might re-color these just so that there's a visual hierarchy of color (e.g. that re-ranked documents take on a different tint) - this way as you are clicking around you can clearly see the difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Re-using the DocumentItem component is good but I think just showing the new score label might be a tad confusing? Or just using score as an abstract is intended here.

Copy link
Contributor

Choose a reason for hiding this comment

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

In one case it's often spacial distance where as when running through a reranker it is a relevance rank. Just thinking that from a user's perspective displaying score: XX alongside both we lose a bit of an opportunity to explain the score in this context a bit better - score being pretty generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think even though score is generic, it is still accurate. On the input side of the reranker, score may or may not exist, and even if it does exist, it's not considered by the reranker. But if the "input" score does exist it was generated by a preprocessor for a separate purpose. The general mental picture here is that there could be millions of documents in a corpus, and only a relatively small set are chosen to be reranked, and that selection process can have a score of its own based on the query in question. Even though that score is not meaningful to the reranker, it is still an informative attribute of the input document, because it relays the reason for how the document became a candidate in the first place (especially when the preprocessor is missing in the trace). On the other hand, we can't really get more specific that the score verbiage because we don't have more information. On balance, although it may seem confusing at first, a user should have enough context to reason their way through it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I wasn't disputing the way we capture the score - was just thinking of ways to avoid the mental "eason their way through it." a bit. But I don't have an immediate good prefix for the reranker score so let's keep it for now.

RERANKING_INPUT_DOCUMENTS,
RERANKING_MODEL_NAME,
RERANKING_OUTPUT_DOCUMENTS,
RERANKING_TOP_K,
Copy link
Contributor

Choose a reason for hiding this comment

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

quick question on this parameter: https://docs.cohere.com/docs/reranking

I'm guessing this is the same as TOP_N? If you feed say 5 documents but pass top_k of 3, does it only rank 3? Just trying to understand why this is a parameter to the rerank model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is the same as TOP_N. (The letter is K in literature because N is usually the total number of docs.) The caller of the reranker usually just wants a relatively small number of docs out of a initial set of tens or hundreds. It's certainly optional because it can just rank each document, but in general, a reduction in number is expected for each stage of the retrieval process.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still confused though - if I pass say 5 documents with top_k of 3 - does it rank 5 and trim the last two?

Copy link
Contributor Author

@RogerHYang RogerHYang Oct 12, 2023

Choose a reason for hiding this comment

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

Yes, it retains up to K in the output, so in the case of top 3, two docs of the lowest scores have to be dropped. Top K is applied after ranking all 5 docs.

@@ -76,11 +75,7 @@
"if not (openai_api_key := os.getenv(\"OPENAI_API_KEY\")):\n",
" openai_api_key = getpass(\"🔑 Enter your OpenAI API key: \")\n",
"openai.api_key = openai_api_key\n",
"os.environ[\"OPENAI_API_KEY\"] = openai_api_key\n",
"\n",
"if not (cohere_api_key := os.getenv(\"COHERE_API_KEY\")):\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the internal one - it seemed nice to have a notebook that executed a reranker?

Copy link
Contributor Author

@RogerHYang RogerHYang Oct 12, 2023

Choose a reason for hiding this comment

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

oh, my bad. I had mistaken this one with the public notebook llama_index_tracing_tutorial.ipynb which has a very similar name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted

Copy link
Contributor

@mikeldking mikeldking left a comment

Choose a reason for hiding this comment

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

@RogerHYang RogerHYang merged commit 1708748 into main Oct 12, 2023
10 checks passed
@RogerHYang RogerHYang deleted the llama-index-reranking branch October 12, 2023 21:10
@github-actions github-actions bot locked and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[trace][semantic] attributes for re-ranking
3 participants