Repository navigation
Implementation/ai 178 crud for embeddingsets - #5
Conversation
An embedding set describes one way of indexing work packages for semantic search: the embedding model, the number of vector dimensions and the context length in tokens, together with a display name and a description for administrators. Sets are standalone records. The state column is in place for the ingestion lifecycle (created, ingesting, error, ready) but only "created" is used so far. The model must be an embedding model offered by the active LLM connection. It is stored by its external identifier and only checked when it changes, so a set keeps working as a record if the server stops offering the model later. https://community.openproject.org/wp/AI-178
Creating, updating and deleting embedding sets goes through the usual BaseServices with contracts, following the labels administration. All three operations are restricted to active administrators. https://community.openproject.org/wp/AI-178
Below the semantic search feature, a new box lists the embedding sets with their model, context length, dimensions and status. Administrators create and edit sets in a dialog and delete them through a danger dialog, opened from a row action menu. The model picker only offers models known to create embeddings. Every set shows "Not ingested" for now; ingestion, activation and locking the parameters of the active set follow separately. The box and the endpoints require both the llm_connection and the semantic_search feature flags. https://community.openproject.org/wp/AI-178
| t.string :title, null: false | ||
| t.text :description | ||
| t.string :model_id, null: false, limit: 512 | ||
| t.integer :embedding_vector_dimensions, null: false | ||
| t.integer :context_token_length, null: false | ||
| t.string :state, null: false, default: "created" |
There was a problem hiding this comment.
I'm not sure if we need title and description.
We could also reference the model directly instead of through model_id?
| def create | ||
| result = ::EmbeddingSets::CreateService | ||
| .new(user: current_user) | ||
| .call(embedding_set_params) | ||
|
|
||
| respond_to_save(result, notice: t(:notice_successful_create)) | ||
| end |
There was a problem hiding this comment.
At the end either the controller or the CreateService would need to schedule the IndexWorkPackagesJob or we would need a separate button for that.
Did you leave that out intentionally? At the moment I can create embedding sets but I can't start the ingestion for them.
There was a problem hiding this comment.
yes, bc I wanted to do the state action in an extra work package. Maybe it makes sense to leave that out entirely here
| def state | ||
| render(Primer::Beta::Text.new(color: :subtle)) do | ||
| I18n.t("admin.embedding_sets.states.#{embedding_set.state}") | ||
| end | ||
| end |
There was a problem hiding this comment.
The state is not readable for me when it's "Not Ingested"
| def update | ||
| result = ::EmbeddingSets::UpdateService | ||
| .new(user: current_user, model: @embedding_set) | ||
| .call(embedding_set_params) | ||
|
|
||
| respond_to_save(result, notice: t(:notice_successful_update)) | ||
| end |
There was a problem hiding this comment.
I think we shouldn't allow to update all the fields of an embedding set? Changing the model, context length and vector dimensions while keeping the embeddings would give us false results.
There was a problem hiding this comment.
At least not when there is already a started ingestion ...
| f.text_field( | ||
| name: :context_token_length, | ||
| type: :number, | ||
| min: 1, | ||
| label: EmbeddingSet.human_attribute_name(:context_token_length), | ||
| caption: I18n.t("admin.embedding_sets.form.context_token_length_caption"), | ||
| required: true, | ||
| input_width: :small | ||
| ) | ||
|
|
||
| f.text_field( | ||
| name: :embedding_vector_dimensions, | ||
| type: :number, | ||
| min: 1, | ||
| label: EmbeddingSet.human_attribute_name(:embedding_vector_dimensions), | ||
| caption: I18n.t("admin.embedding_sets.form.embedding_vector_dimensions_caption"), | ||
| required: true, | ||
| input_width: :small | ||
| ) |
There was a problem hiding this comment.
Here you could probably prefill context length and dimensions with the model settings if there are any
| <% @features.each do |feature| %> | ||
| <%= render( | ||
| LlmConnections::FeatureBindingComponent.new( | ||
| feature:, | ||
| connection: @connection, | ||
| binding: @bindings[feature.key.to_s] | ||
| ) | ||
| ) %> | ||
| <% end %> | ||
|
|
||
| <% if OpenProject::FeatureDecisions.semantic_search_active? %> | ||
| <%= render(LlmConnections::EmbeddingSets::BoxComponent.new(embedding_sets: EmbeddingSet.order(:created_at))) %> | ||
| <% end %> |
There was a problem hiding this comment.
I think at the end the embedding set component should probably also be a feature binding component to stay aligned with that architecture.
There was a problem hiding this comment.
yes, agreed. My roadmap would be:
- Dummy States
- Real ingestions
- Wiring it together with your work and getting rid of the old feature binding fields in favour of a fk on EmbeddingSets (or SemanticIndexes)
What are you trying to accomplish?
Admins can create, edit and delete embedding sets in a new box below "Semantic search" on the LLM connection's Feature configuration tab. Each set stores the model, context length (tokens), vector dimensions, a title and a description. It's the first step towards preparing several embedding configurations, ingesting them separately and activating one.
Out of scope for this PR: ingestion, state transitions, activation, and locking the active set. A
statecolumn exists, but it is alwayscreatedand shown as "Not ingested".Screenshots
What approach did you choose and why?
EmbeddingSetthat belongs to nothing. A feature binding will point to the active set later.model_idis stored as a string, like on the binding, so a set survives a model being removed. The model has to be one the active connection offers for embeddings.BaseServicesand contracts (labels pattern), restricted to admins.BorderBoxTable,ActionMenu,DangerDialog), behind thellm_connectionandsemantic_searchfeature flags.AI involvement
Merge checklist