-
Notifications
You must be signed in to change notification settings - Fork 232
DRAFT: Added an updated version of notebook for video script #471
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
base: main
Are you sure you want to change the base?
Conversation
Found 1 changed notebook. Review the changes at https://app.gitnotebooks.com/elastic/elasticsearch-labs/pull/471 |
@justincastilla I see you've added a new notebook rather than updated the existing one. Why is that? I would have expected us to update the existing notebook, especially if the plan is to update the piece as well. Can you also double check if things are running as expected and using latest possible versions? I see an error in one of the cells in your updated notebook:
|
I'm not going to replace it fully until this new notebook is approved and ready. Also, we'll want it around until the article is updated, as it references the same code.
The new code no longer needs those packages. It's been removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the summary being added. Can you make sure all TODO's are addressed and the comments removed? After that I think we're good.
As discussed on Wednesday, let's also make sure this updated notebook replaces the original once the piece has been updated.
"# Index to load products-ecommerce.json docs\n", | ||
"if client.indices.exists(index=\"ecommerce\"):\n", | ||
" client.indices.delete(index=\"ecommerce\")\n", | ||
"We define the `e5_description_vector` and the `elser_description_vector` fields to store the inference pipeline results. The field type in `e5_description_vector` is a `dense_vector`. The `.e5_multilingual_small` model has embedding_size of 384, so the dimension of the fector (dims) is set to 384. \n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"We define the `e5_description_vector` and the `elser_description_vector` fields to store the inference pipeline results. The field type in `e5_description_vector` is a `dense_vector`. The `.e5_multilingual_small` model has embedding_size of 384, so the dimension of the fector (dims) is set to 384. \n", | |
"We define the `e5_description_vector` and the `elser_description_vector` fields to store the inference pipeline results. The field type in `e5_description_vector` is a `dense_vector`. The `.e5_multilingual_small` model has embedding_size of 384, so the dimension of the vector (dims) is set to 384. \n", |
"source": [ | ||
"# Performs text analysis on a string and returns the resulting tokens.\n", | ||
"\n", | ||
"# TODO: Partial Smoosh together\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the TODOs (there's a couple in this file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's still 1 TODO comment in there, but aside from that LGTM.
Updated the notebook to use modern mechanisms and API calls. I ONLY updated the notebook codebase, not the supporting text. I will change the copy at a later time