GetCachedModels to return valid and accurate entries - #996
GetCachedModels to return valid and accurate entries#996Baiju Meswani (baijumeswani) wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Updates cached-model enumeration to scan disk directly without requiring catalog availability.
Changes:
- Enumerates all valid locally cached variants.
- Enriches entries from cached catalog metadata with disk-only fallback.
- Adds unit and cache-only integration coverage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
sdk_v2/cpp/src/catalog/azure_model_catalog.cc |
Implements disk-based cached-model enumeration. |
sdk_v2/cpp/src/catalog/azure_model_catalog.h |
Adds cached-model storage and synchronization. |
sdk_v2/cpp/test/internal_api/local_model_scanner_test.cc |
Tests variant enumeration and metadata fallback. |
sdk_v2/cpp/test/sdk_api/cache_only_test.cc |
Verifies cached models without catalog access. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
390d2b1 to
fa977b1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (5)
sdk_v2/cpp/src/catalog/base_model_catalog.cc:174
- Calling
GetCachedModels()after catalog population also resets all explicitSelectVariant()choices, even though cached-model enumeration should not mutate user selection. Re-sort to reflect refreshed metadata, but only update selection when the container is still using an automatically chosen default.
for (auto& model : models_) {
model->SortVariants();
model->SelectDefaultVariant();
}
sdk_v2/cpp/src/model.cc:445
- The early return for an already-cached model prevents a rescan from correcting a changed cache directory. This affects models without snapshot metadata, so
GetCachedModels()can return a canonical model whoseGetPath()still names the removed old directory. Compare and publish the newly scanned path instead of treatingcached_as proof that the path is current.
void Model::RefreshCachedPath(std::string local_path) {
if (local_path.empty() || cached_.load(std::memory_order_acquire)) {
return;
sdk_v2/cpp/src/catalog/base_model_catalog.cc:174
- A complete rescan never clears cache state for canonical IDs that disappeared from
scanned_ids. If another process removes or invalidates a model directory,GetCachedModels()omits it, but previously returned pointers—andGetModelVariant()after population—still reportIsCached() == truewith the stale path. Reconcile absent previously-cached canonical leaves by publishing an empty path and clearing their cached flag.
if (populated_) {
for (auto& model : models_) {
model->SortVariants();
model->SelectDefaultVariant();
}
sdk_v2/cpp/src/model.cc:440
- This only publishes the scanned path when the model was previously uncached. If the same model ID is rediscovered at a different directory after its old directory is moved or replaced, the canonical object returned by
GetCachedModels()keeps the stale path. Publish an existing path whenever it differs from the current snapshot, even whencached_is already true.
This issue also appears on line 443 of the same file.
if (!local_path.empty() && !cached_.load(std::memory_order_acquire) && std::filesystem::exists(local_path)) {
PublishLocalPath(std::move(local_path));
cached_.store(true, std::memory_order_release);
}
sdk_v2/cpp/src/catalog/base_model_catalog.cc:114
- This resets every existing container to its default variant on each catalog integration, including integrations that only refresh metadata. A variant explicitly chosen through the public
SelectVariant()API is therefore silently replaced after a refresh. Preserve explicit selections while re-sorting; track whether selection is user-chosen and only recompute the default for new/default-selected containers.
This issue also appears on line 171 of the same file.
for (auto& model : models_) {
model->SortVariants();
model->SelectDefaultVariant();
fa977b1 to
bac6042
Compare
| virtual std::string_view GetName() const = 0; | ||
| virtual ModelList GetModels() const = 0; | ||
|
|
||
| /// Get every leaf model variant currently present in the local cache. |
There was a problem hiding this comment.
'leaf' isn't a concept a C++ API user knows anything about.
| } | ||
| } | ||
| auto catalog_result = GetLiveCatalogOrLocalSnapshot(cached_model_ids); | ||
| auto models = AddLocalModels(catalog_result.model_infos, local_models); |
There was a problem hiding this comment.
If catalog_result is from a snapshot do we need to call AddLocalModels?
I guess a new local model could have been added post-snapshot. Most likely it's just a bunch of de-duping to do from the call.
| models.reserve(model_infos.size()); | ||
| for (const auto& info : model_infos) { | ||
| auto local_model = local_models.find(info.model_id); | ||
| auto local_path = local_model != local_models.end() ? local_model->second : std::string{}; | ||
| models.push_back(model_factory_(ModelInfo(info), std::move(local_path))); | ||
| } |
There was a problem hiding this comment.
Can we do this at the same point we add the local model to model_infos?
This PR makes cached-model discovery resilient to catalog outages and returns every cached model variant instead of one selected model per alias.
foundry.modelinfo.jsonwhen all live catalog URLs fail.Issues addressed
Offline catalog returned no cached models
foundry.modelinfo.jsonwere present locally.Locally valid models missing from metadata were omitted
Only one cached variant per alias was returned
GetCachedModels()returned the alias container, which exposed only its selected CPU/GPU/NPU variant.Cache-only mode did not reconcile snapshot metadata with disk state
Multiple catalog URLs could produce duplicate model IDs
Cached-model results could hide the canonical variant objects
GetModelVariant(model_id).Code flow