-
Notifications
You must be signed in to change notification settings - Fork 788
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: add online serving w/multiple entities notebook #3245
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
7f77645
to
21b66c0
Compare
21b66c0
to
edd3240
Compare
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.
Added a few nits. Main (global) feedback:
- Avoid the use of the first person (I/we).
- Avoid directional text such as "below". Preferred: "following".
- Having a description before a code sample can help improve usability.
"## Overview\n", | ||
"\n", | ||
"This notebook demonstrates how to fetch multiple entities (and their feature\n", | ||
"values) in real-time. This notebook introduces the *StreamingFetchFeatureValues* (SFFV) API for faster multi-entity lookup.\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.
I wonder if it's okay for us to surface the "SFFV" in the docs. Has its use in public artifacts been approved by Alex/Raiyaan?
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.
For now Raiyaan has okayed usage. Will follow up with Alex.
}, | ||
"source": [ | ||
"<div class=\"alert alert-block alert-warning\">\n", | ||
"<b>⚠️ The kernel is going to restart. Wait until it's finished before continuing to the next step. ⚠️</b>\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.
I'm unfamiliar with this annotation. How is this rendered?
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.
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.
Is this a standard text or can we shorten it to, say, "Wait for the kernel to restart before continuing to the next step"?
Also, I feel we should render this as a "Caution:" such as what's illustrated here i.e. without the icons: https://developers.google.com/style/notices
However, please feel free to ignore these comments if this is a standard note that's been used elsewhere.
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.
This is standard from the template - https://github.com/GoogleCloudPlatform/vertex-ai-samples/blob/main/notebooks/notebook_template.ipynb. @gericdong to comment if this should be changed to caution?
notebooks/official/feature_store/online_feature_serving_fetching_multiple_entities.ipynb
Show resolved
Hide resolved
notebooks/official/feature_store/online_feature_serving_fetching_multiple_entities.ipynb
Show resolved
Hide resolved
notebooks/official/feature_store/online_feature_serving_fetching_multiple_entities.ipynb
Outdated
Show resolved
Hide resolved
notebooks/official/feature_store/online_feature_serving_fetching_multiple_entities.ipynb
Outdated
Show resolved
Hide resolved
notebooks/official/feature_store/online_feature_serving_fetching_multiple_entities.ipynb
Outdated
Show resolved
Hide resolved
notebooks/official/feature_store/online_feature_serving_fetching_multiple_entities.ipynb
Outdated
Show resolved
Hide resolved
notebooks/official/feature_store/online_feature_serving_fetching_multiple_entities.ipynb
Outdated
Show resolved
Hide resolved
notebooks/official/feature_store/online_feature_serving_fetching_multiple_entities.ipynb
Outdated
Show resolved
Hide resolved
98cb371
to
9b7dcbd
Compare
9b7dcbd
to
ac33b43
Compare
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.
Just one nit. LGTM otherwise!
}, | ||
"source": [ | ||
"<div class=\"alert alert-block alert-warning\">\n", | ||
"<b>⚠️ The kernel is going to restart. Wait until it's finished before continuing to the next step. ⚠️</b>\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.
Is this a standard text or can we shorten it to, say, "Wait for the kernel to restart before continuing to the next step"?
Also, I feel we should render this as a "Caution:" such as what's illustrated here i.e. without the icons: https://developers.google.com/style/notices
However, please feel free to ignore these comments if this is a standard note that's been used elsewhere.
REQUIRED: Add a summary of your PR here, typically including why the change is needed and what was changed. Include any design alternatives for discussion purposes.
add online serving w/multiple entities notebook - introduces StreamingFetchFeatureValues API
REQUIRED: Fill out the below checklists or remove if irrelevant
Official Notebooks
under the notebooks/official folder, follow this mandatory checklist:Official Notebooks
section, pointing to the author or the author's team.Community Notebooks
under the notebooks/community folder:Community Notebooks
section, pointing to the author or the author's team.Community Content
under the community-content folder:Content Directory Name
is descriptive, informative, and includes some of the key products and attributes of your content, so that it is differentiable from other contentCommunity Content
section, pointing to the author or the author's team.