-
Notifications
You must be signed in to change notification settings - Fork 330
Integrate "get model versions" and "download specific model version" into cpp Core with max_versions #816
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
Open
selenayang888
wants to merge
24
commits into
main
Choose a base branch
from
syang/integrate-get-model-versions-into-cpp
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,297
−123
Open
Integrate "get model versions" and "download specific model version" into cpp Core with max_versions #816
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
3a6155b
Enable live catalog with region-aware fallback engine
baijumeswani cc1e22f
Add AzureAiStudio User-Agent
baijumeswani a827ec8
Address pr review comments
baijumeswani a75bb36
integrate get model versions into cpp core
selenayang888 fd865f3
Default registry region to centralus, httprequestoptions, and config …
baijumeswani c872980
Merge branch 'main' of https://github.com/microsoft/Foundry-Local int…
baijumeswani 22f5b0f
Address pull-request review comments
baijumeswani d4b61f3
Address pull-request review comments
baijumeswani 6739874
add max_version and continuation token
selenayang888 f0fb73c
Change all defaults to centralus
baijumeswani 859d927
Address pull-request review comments
baijumeswani 7a3d089
add a new test with empty alias and sort the results
selenayang888 b73c5e1
Merge remote-tracking branch 'origin/baijumeswani/catalog' into syang…
selenayang888 e7802c7
Merge remote-tracking branch 'origin/main' into syang/integrate-get-m…
selenayang888 2a37f65
Resolve all the comments
selenayang888 ffb7f46
put "CompareCaseInsensitive" in utils/string_utils.h
selenayang888 850358e
Merge remote-tracking branch 'origin/main' into syang/integrate-get-m…
selenayang888 1a31fc3
Resolved three comments
selenayang888 7164318
Resolve two comments
selenayang888 e9aa2f4
Resolved `GetModelVersions` , local copy and refactor `catalog_urls_ `
selenayang888 e4401ce
Addressing the comment for ordering of `CompareModelPointersForVersio…
selenayang888 94a1796
Merge remote-tracking branch 'origin/main' into syang/integrate-get-m…
selenayang888 bbfb6b4
Align with CompareModelsForSort
selenayang888 2f9d1d2
Resolved "Replacing on each call creates potential issues on the clie…
selenayang888 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Should we note that (IIUC) these models will be returned by catalog operations in the current instance (e.g. list models) but are not saved in the model cache on disk so won't be present on restart? I think that behavior is fine but it could be a little surprising.
Uh oh!
There was an error while loading. Please reload this page.
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 this is how design doc specified for caching strategy.
get_model_versionsresults are not cached inBaseModelCatalog. Each call toget_model_versionsdoes a fresh catalog query. However, to supportdownloadan older version", the resolved ModelInfo for that version is temporarily added to the catalog's modelIdToInfo index.The link below is the part in the design doc for Caching Strategy for All-Versions Data:
model-versions-design.md
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.
should we explain that behavior to the user in the comments here as it's a little opaque?
Uh oh!
There was an error while loading. Please reload this page.
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 the explanation of the behavior in the comments now.
Moreover, based on the design doc requirements for caching strategy, I updated the
GetModelVersionsinbase_model_catalog.cc:475no longer callsIntegrateVariants. It now stores the fetched models in transient per-catalog query storage and returns those handles without adding them to the main alias/id indices.