embedding content updates and removals #2217
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
OpenAPI ChangesShow/hide 16 changes: 0 error, 0 warning, 16 info |
OpenAPI ChangesShow/hide 16 changes: 0 error, 0 warning, 16 info |
OpenAPI ChangesShow/hide 16 changes: 0 error, 0 warning, 16 info |
OpenAPI ChangesShow/hide 16 changes: 0 error, 0 warning, 16 info |
OpenAPI ChangesShow/hide 16 changes: 0 error, 0 warning, 16 info |
abeglova
requested changes
May 1, 2025
| operations = [ | ||
| migrations.AddField( | ||
| model_name="contentfile", | ||
| name="archive_checksum", |
There was a problem hiding this comment.
Can you add a task to the migration to set archive_checksum to the current value of checksum and calculate the checksum from the content for existing contentfiles
learning_resources/serializers.py
Outdated
| @@ -1286,13 +1288,15 @@ class Meta: | |||
| "uid", | |||
| "title", | |||
| "description", | |||
| "archive_checksum", | |||
There was a problem hiding this comment.
You don't need archive_checksum in the serializer since it's not useful information for users or used for elasticsearch/vector search
| unpublished_tasks = [ | ||
| tasks.bulk_deindex_learning_resources.si(ids, resource_type), | ||
| ] | ||
| if django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS: |
There was a problem hiding this comment.
I think the elasticsearch tasks should be chained before the vector embedding tasks since the vector embedding tasks can potentially take a long time. It's better to make the elasticsearch index up to date sooner
vector_search/utils.py
Outdated
| based on the resource_type (learning resource vs content file) | ||
| """ | ||
| client = qdrant_client() | ||
| if resource_type != CONTENT_FILE_TYPE: |
There was a problem hiding this comment.
Does it make sense to have a seperate version of update_payload(), should_generate_embeddings(), _embedding_context() etc for learning resources and content files instead of having all the if statements?you should be able to know which version of the function to call in the task
vector_search/utils.py
Outdated
| ) | ||
|
|
||
|
|
||
| def should_generate_embeddings(serialized_document, resource_type): |
There was a problem hiding this comment.
I think it makes sense to have seperate content file and learning resource verisons of this function instead of all the if statements.
You can consider separate content file or resource specific versions of update_payload(), and _embedding_context() too
There was a problem hiding this comment.
good idea. I updated the code so they each have their own _embedding_context, update_payload and should_generate_embeddings methods
OpenAPI ChangesShow/hide 16 changes: 0 error, 0 warning, 16 info |
OpenAPI ChangesShow/hide 16 changes: 0 error, 0 warning, 16 info |
OpenAPI ChangesShow/hide 16 changes: 0 error, 0 warning, 16 info |
OpenAPI ChangesShow/hide 16 changes: 0 error, 0 warning, 16 info |
OpenAPI ChangesShow/hide 16 changes: 0 error, 0 warning, 16 info |
OpenAPI ChangesShow/hide 8 changes: 0 error, 0 warning, 8 info |
OpenAPI ChangesShow/hide 8 changes: 0 error, 0 warning, 8 info |
OpenAPI ChangesShow/hide 8 changes: 0 error, 0 warning, 8 info |
OpenAPI ChangesShow/hide 8 changes: 0 error, 0 warning, 8 info |
This was referenced May 6, 2025
Closed
Closed
Closed
Closed
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/6754
Description (What does it do?)
This PR introduces updates/upserts and removals for embedding vectors.
The main changes:
For contentfiles
For resources
How can this be tested?
testing the etl hooks
python manage.py backpopulate_micromasters_dataFor reference, here is a list of things that this PR (and functionality before it) should ensure: