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

ET-4885 allow to prefix snippets #1107

Merged
merged 4 commits into from
Nov 15, 2023
Merged

Conversation

rustonaut
Copy link
Contributor

Allows specifying two prefixes:

  • embedding.prefix.query prefix all queries with this before computing their embedding
  • embedding.prefix.snippet prefix all snippets with this before computing their embedding

works for both local and sagemaker setups

Env variables:

  • XAYN_WEB_API__EMBEDDING__PREFIX__QUERY
  • XAYN_WEB_API__EMBEDDING__PREFIX__SNIPPET

@github-actions github-actions bot added the ready-for-review The PR can be reviewed label Nov 14, 2023
};

let sequence = if let Some(prefix) = prefix {
Cow::Owned(format!("{prefix} {sequence}"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the concatenation using a space okay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

#[cfg_attr(test, serde(deny_unknown_fields))]
pub(crate) struct Prefix {
/// Prefix prepended to search queries when embedding them.
pub(crate) query: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if we care to distinguish between prefix not given and an empty prefix. The end result will be the same.

Copy link
Contributor Author

@rustonaut rustonaut Nov 15, 2023

Choose a reason for hiding this comment

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

If you by default separate the prefix with a white-space from the rest it makes a difference.

Through we removed the default space separator so I now changed it to empty string.

@rustonaut rustonaut added this pull request to the merge queue Nov 15, 2023
Merged via the queue into main with commit 782e49e Nov 15, 2023
7 checks passed
@rustonaut rustonaut deleted the ET-4885-allow-to-prefix-snippets branch November 15, 2023 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review The PR can be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants