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: allow custom metadata key #1

Merged
merged 1 commit into from
May 20, 2024
Merged

Conversation

ogzhanolguncu
Copy link
Contributor

@ogzhanolguncu ogzhanolguncu commented May 13, 2024

This PR allows:

  • Users to pass new metadata keys
  • sessionTTL
  • When adding context allows them to pass id,vector|data and metadata. When user just passes input as plain text we also put that into metadata field, but if user explicitly passes metadata, plain text will be overwritten.

Comment on lines +75 to +79
const metadata = context.metadata
? { [metadataKey]: context.metadata }
: isText
? { [metadataKey]: context.input }
: {};
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 the first condition is not correct, metadata will be like {"text": {"genre": "comedy"}}

I think there should be 4 cases:

isText !isText
context.metadata { [key]: input, ...context.metadata } context.metadata
!context.metadata { [key]: input } {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{ [metadataKey]: context.metadata } this is same as {value: "hello world"} but key is dynamic and default is text. And, also, if user is passing plain text instead of vector and doesn't pass metadata field we'll fallback to default context.input which is plaintext field.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hadn't realised that metadata is a string field. I guess this allows separating synthesis and retrieval texts.

I feel like people may want to include other fields other than the text itself for filtered retrieval. But I am okay with leaving this for later since it's not a must for the first release

@ogzhanolguncu ogzhanolguncu merged commit ef23aab into master May 20, 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.

2 participants