fix: Deployments should be cached for non-model-information #102
Conversation
This file contains 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
marikaner
force-pushed
the
cache
branch
5 times, most recently
from
cf0345e
to
d9f2959
Compare
marikaner
commented
Sep 3, 2024
| [...deployments].reverse().forEach(deployment => { | ||
| cache.set(getCacheKey({ ...opts, model: extractModel(deployment) }), { | ||
| entry: transformDeploymentForCache(deployment) | ||
| const cacheEntries = [...deployments].map(deployment => |
There was a problem hiding this comment.
@MatKuhr how do you like that?
my 2 cents: I think it is semantically better, however more error prone.
There was a problem hiding this comment.
Ok, after multiple tries and still failing tests, I am reverting this. Feel free to check the commit, if you are interested: 37eaadd
This reverts commit 37eaadd.
marikaner
commented
Sep 3, 2024
Comment on lines
61
to
72
| .flatMap(entry => { | ||
| const entries = [entry]; | ||
| // if the entry has a model, also cache it without model and version | ||
| if (entry.model) { | ||
| entries.push({ id: entry.id }); | ||
| } | ||
| // if the entry has a model version, also cache it for without model version | ||
| if (entry.model?.version) { | ||
| entries.push({ id: entry.id, model: { name: entry.model.name } }); | ||
| } | ||
| return entries; | ||
| }); |
There was a problem hiding this comment.
This could still be shortened if we remove the checks:
Suggested change
| .flatMap(entry => { | |
| const entries = [entry]; | |
| // if the entry has a model, also cache it without model and version | |
| if (entry.model) { | |
| entries.push({ id: entry.id }); | |
| } | |
| // if the entry has a model version, also cache it for without model version | |
| if (entry.model?.version) { | |
| entries.push({ id: entry.id, model: { name: entry.model.name } }); | |
| } | |
| return entries; | |
| }); | |
| .flatMap(entry => [entry, { id: entry.id }, { id: entry.id, model: { name: entry.model.name } }]); |
Even though this is a little less performant, I think I even prefer that, but no strong opinion.
# Conflicts: # packages/gen-ai-hub/src/utils/deployment-cache.ts
MatKuhr
approved these changes
Sep 4, 2024
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.
Context
AI/gen-ai-hub-sdk-js-backlog#78.
Follow up from #96.
When retrieving a deployment for a model name without version and there are only deployments where each model has a version, the second time we use this deployment, the cache wasn't used.
Definition of Done