Allow users to pass customField to /rerank endpoint #303

aulorbe · 2024-10-15T22:22:19Z

Problem

The current implementation of /rerank in the TS client does not (correctly) allow users to pass a custom field upon which to rerank.

Solution

Allow custom fields!

Please reference this PR to account for all expected functionality: https://github.com/pinecone-io/python-plugin-inference/pull/21/files

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

CI passes + reviewer xreferences PR above w/functionality introduced in this PR.

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1208523729730914

This reverts commit d3b7f0e.

austin-denoble

LGTM overall, I did leave some question and comments about a few things. I feel like I haven't read much TypeScript in a while and am a bit rusty. 😅

austin-denoble · 2024-10-16T22:28:15Z

src/inference/inference.ts

+      } else {
+        throw new PineconeArgumentError(
+          'Documents must be a list of strings or a list of objects'
+        );
+      }


Is it possible for it to be a mix of the two? I'm actually not sure, but I'm assuming we don't allow that.

Ah that's a good q! Right now, the TS client doesn't account for that. I'll add it in, thank you!

Do you mean if someone mixed strings and documents in the same request? That would be kinda odd, I don't think we need to bend over backwards to support that

okay cool. I think my final else stmt would actually account for this anyways, no?

The logic is:

if docs is an array of strings, do something

else if docs is an array of objs, do something else

else, error out

Yeah, I think the logic should be fine for that atm.

austin-denoble · 2024-10-16T22:31:50Z

src/inference/inference.ts

+      if (documents.every((doc) => typeof doc === 'string')) {
+        // Set rankFields to default value of 'text'
+        computedRankFields = ['text'];
+      } else if (
+        // If`documents` is an array of objects:
+        documents.every((doc) => typeof doc === 'object' && doc !== null)
+      ) {
+        // If there is a `text` key (other keys can be present too):
+        if (
+          documents.every((doc) => (doc as { [key: string]: string })['text'])


Is there maybe a more efficient way to parse the documents that are passed and make some determinations about the contents one time up front rather than calling documents.every so many times? In the last code path we'd be iterating over everything in the array 3 times which is a bit heavy.

I'd try maybe looping over it once and building some parameters/bools that we use in these if statements rather than iterating repeatedly.

austin-denoble · 2024-10-16T22:32:13Z

src/inference/inference.ts

+        computedRankFields = ['text'];
+      } else if (
+        // If`documents` is an array of objects:
+        documents.every((doc) => typeof doc === 'object' && doc !== null)


Why do we check doc !== null here but not with the string case above?

Added above!

austin-denoble · 2024-10-16T22:39:02Z

src/inference/inference.ts

-    }
+    // Standardize documents to ensure they are in object format
+    const newDocuments = documents.map((doc) =>
+      typeof doc === 'string' ? { text: doc } : doc


Will the text key here be different if they passed a custom rankFields?

I don't think that's a valid scenario, here's my thinking:

If someone passes in an array of strings, there are no keys in their documents

If there are no keys in their documents, on the backend we artificially transform their documents into an array of key:value pairs, with a text key, simply b/c that's what the model needs for reranking

If someone passes in an array of strings and then also passes in a custom field to rerank on... their docs would inherently be missing that custom field (by way of not being key:value pairs), so it would error out

Thoughts @ssmith-pc ?

I'm not fully following the original question since the impl has changed, but:

If there are no keys in their documents, on the backend we artificially transform their documents into an array of key:value pairs, with a text key, simply b/c

By "on the backend we transform" I think you mean, it's transformed in the client right? This is the right behavior

If someone passes in an array of strings and then also passes in a custom field to rerank on... their docs would inherently be missing that custom field (by way of not being key:value pairs), so it would error out

Correct, but it's fine if that error is detected in the server and sent back to the client. The client doesn't have to try and detect that.

austin-denoble · 2024-10-16T22:39:56Z

src/inference/__tests__/embed.test.ts

+    const params = { inputType: 'text', truncate: 'END' };
+    const expected = { inputType: 'text', truncate: 'END' };


Do we need to test if we're not expecting this to change anything? Maybe I'm unclear on the test.

weird... this must be a holdover from something... I'll remove method (it does nothing currently) + this test. thanks!

austin-denoble · 2024-10-16T22:44:03Z

src/integration/inference/rerank.test.ts

+      { text: 'hi', customField: 'doc1' },
+      { text: 'bye', customField: 'doc2' },
+    ];
+    expect(pinecone.inference.rerank(model, query, myDocuments)).toBeDefined();


I'm a little rusty on ts at this point, if the call fails it would avoid the expect and fail the test, right? Just trying to understand what this is asserting.

oh man, extremely good catch! I'll change to assert that rerankUnits are >= 1

austin-denoble · 2024-10-16T22:45:27Z

src/inference/__tests__/rerank.test.ts

+  const expectedError = new PineconeArgumentError(
+    'You must pass at least one document to rerank'
+  );


nit: Personal preference, but I'd maybe just leave the expected error inline on the expect().toEqual(). Feels easier to parse.

jseldess

Looks good to me, @aulorbe, but can you add an example to the README?

aulorbe · 2024-10-17T17:14:18Z

Looks good to me, @aulorbe, but can you add an example to the README?

Thank you!

I think the existing example in the README on the RC branch is still valid, right? Or were you think I should add more to that one?

jseldess · 2024-10-17T17:29:31Z

Looks good to me, @aulorbe, but can you add an example to the README?

Thank you!

I think the existing example in the README on the RC branch is still valid, right? Or were you think I should add more to that one?

Yeah, I think it's still valid. But perhaps we should add rankFields to the second example. Also, I'm not sure if it's worth NOT setting the other rank options in the first example (top_n, return documents, etc.). I'd suggest the first example has everything but rankfields, and the second example shows how to use rankfields to rank on a custom field.

aulorbe · 2024-10-17T18:19:41Z

Looks good to me, @aulorbe, but can you add an example to the README?

Thank you!
I think the existing example in the README on the RC branch is still valid, right? Or were you think I should add more to that one?

Yeah, I think it's still valid. But perhaps we should add rankFields to the second example. Also, I'm not sure if it's worth NOT setting the other rank options in the first example (top_n, return documents, etc.). I'd suggest the first example has everything but rankfields, and the second example shows how to use rankfields to rank on a custom field.

Cool, will do!

jseldess

README LGTM, with a nit

README.md

Co-authored-by: Jesse Seldess <j_seldess@hotmail.com>

ssmith-pc · 2024-10-17T19:55:24Z

src/inference/inference.ts

-   * should to rank the documents, and additional model-specific parameters. See {@link RerankOptions} for more details.
+   * @param documents - (Required) An array of documents to rerank. The array can either be an array of strings or
+   * an array of objects.
+   * @param options - (Optional) Additional options to send with the reranking request. See {@link RerankOptions} for more details.
   * */
  @prerelease('2024-10')


Should we remove this pre-release tag now

ssmith-pc · 2024-10-17T20:01:58Z

src/inference/inference.ts

-        });
-      });
+    if (!options.rankFields) {
+      // If each obj in newDocs does not have a 'text' field, throw an error


nit: the comment here should probably read "if any obj does not have... "

But may be more clear as simply:
"Ensure each obj in newDocs has a 'text' field"

ssmith-pc · 2024-10-17T20:10:28Z

src/inference/inference.ts

+      if (
+        !newDocuments.every((doc) => typeof doc === 'object' && 'text' in doc)
+      ) {


The server will validate that the documents contain the fields they're supposed to, so it's not entirely necessary to validate on the client. However, if we want to validate on the client, why not also validate the case where a custom rank field was provided?

oh yeah good call, I remove this part actually (the validation part)

ssmith-pc · 2024-10-17T20:11:29Z

src/inference/inference.ts

      parameters = {},
+      rankFields = undefined,


Do we need to destructure rankFields and set to undefined? Isn't it undefined by default? I think I don't entirely understand why we can't keep the rankFields = ['text'] default

Ah good call, good call

austin-denoble

LGTM, thanks for iterating and helping me iterate in return! 🚢

austin-denoble · 2024-10-18T19:43:23Z

src/inference/inference.ts

    const {
      topN = documents.length,
      returnDocuments = true,
-      rankFields = ['text'],


I think this should be left in, right? If they haven't provided a rankFields array explicitly, we should just default to "text".

ssmith-pc

Looks good!

aulorbe added 2 commits October 15, 2024 15:20

Reorganization of files + add tests for rerank w/custom fields

968bf88

Remove logging line

206c8f9

aulorbe marked this pull request as ready for review October 15, 2024 22:27

aulorbe requested review from austin-denoble, ssmith-pc and jseldess October 15, 2024 22:27

aulorbe added 2 commits October 16, 2024 14:46

Try workflow w/new app

d3b7f0e

Revert "Try workflow w/new app"

2ca85a9

This reverts commit d3b7f0e.

austin-denoble approved these changes Oct 16, 2024

View reviewed changes

jseldess reviewed Oct 17, 2024

View reviewed changes

Incorporate feedback

c4e3a73

aulorbe added 2 commits October 17, 2024 11:57

Update README

870451e

Simplify conditional loops

90ee471

aulorbe requested review from austin-denoble and jseldess October 17, 2024 19:16

jseldess approved these changes Oct 17, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

aulorbe and others added 2 commits October 17, 2024 12:22

Update docstring to reflect new example in README

74e5cd9

Update README.md

6803c8e

Co-authored-by: Jesse Seldess <j_seldess@hotmail.com>

ssmith-pc reviewed Oct 17, 2024

View reviewed changes

Address last comments

4d0f115

austin-denoble approved these changes Oct 18, 2024

View reviewed changes

ssmith-pc approved these changes Oct 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to pass customField to /rerank endpoint #303

Allow users to pass customField to /rerank endpoint #303

aulorbe commented Oct 15, 2024 •

edited

Loading

austin-denoble left a comment

austin-denoble Oct 16, 2024

aulorbe Oct 17, 2024

ssmith-pc Oct 17, 2024

aulorbe Oct 17, 2024

austin-denoble Oct 17, 2024

austin-denoble Oct 16, 2024

austin-denoble Oct 16, 2024

aulorbe Oct 17, 2024

austin-denoble Oct 16, 2024

aulorbe Oct 17, 2024

ssmith-pc Oct 17, 2024

austin-denoble Oct 16, 2024

aulorbe Oct 17, 2024

austin-denoble Oct 16, 2024

aulorbe Oct 17, 2024

austin-denoble Oct 16, 2024

aulorbe Oct 17, 2024

jseldess left a comment

aulorbe commented Oct 17, 2024 •

edited

Loading

jseldess commented Oct 17, 2024

aulorbe commented Oct 17, 2024

jseldess left a comment

ssmith-pc Oct 17, 2024

aulorbe Oct 17, 2024

ssmith-pc Oct 17, 2024

ssmith-pc Oct 17, 2024

aulorbe Oct 18, 2024

ssmith-pc Oct 17, 2024

aulorbe Oct 18, 2024

austin-denoble left a comment

austin-denoble Oct 18, 2024

ssmith-pc left a comment

		const params = { inputType: 'text', truncate: 'END' };
		const expected = { inputType: 'text', truncate: 'END' };

Allow users to pass customField to /rerank endpoint #303

Are you sure you want to change the base?

Allow users to pass customField to /rerank endpoint #303

Conversation

aulorbe commented Oct 15, 2024 • edited Loading

Problem

Solution

Type of Change

Test Plan

austin-denoble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseldess left a comment

Choose a reason for hiding this comment

aulorbe commented Oct 17, 2024 • edited Loading

jseldess commented Oct 17, 2024

aulorbe commented Oct 17, 2024

jseldess left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austin-denoble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssmith-pc left a comment

Choose a reason for hiding this comment

aulorbe commented Oct 15, 2024 •

edited

Loading

aulorbe commented Oct 17, 2024 •

edited

Loading