From 274b3e5ab8ebd07829c1933b2a232283ad425f82 Mon Sep 17 00:00:00 2001 From: Scott McKay Date: Sat, 15 Aug 2026 11:10:17 +1000 Subject: [PATCH 1/4] Add plan for supporting multiple catalogs better. --- sdk_v2/cpp/docs/MultiCatalogSupportPlan.md | 397 +++++++++++++++++++++ 1 file changed, 397 insertions(+) create mode 100644 sdk_v2/cpp/docs/MultiCatalogSupportPlan.md diff --git a/sdk_v2/cpp/docs/MultiCatalogSupportPlan.md b/sdk_v2/cpp/docs/MultiCatalogSupportPlan.md new file mode 100644 index 000000000..4333de5ed --- /dev/null +++ b/sdk_v2/cpp/docs/MultiCatalogSupportPlan.md @@ -0,0 +1,397 @@ +# Multi-Catalog Support — Consolidated Plan + +> Status: **Proposal for review** +> Scope: `sdk_v2/cpp` catalog subsystem +> Supersedes: `MultiCatalogAggregationPlan.md` and `MultiSourceCatalogDesign.md` +> (kept for history; do not edit) + +## Summary + +Refactor the catalog subsystem so multiple catalog types — **Public** (Azure) and, as future +follow-ups, **Private** (an additional online catalog source) and **BYOM local** — feed **one +aggregated `ICatalog`**. The public API (`ICatalog` / `flCatalogApi`) stays byte-for-byte identical; +the multiple contributing catalogs are an implementation detail. + +**Initial scope is the Public (Azure) source only.** Locally-cached models are still surfaced, but +**not via a separate source**: the Azure source keeps today's behavior of scanning the cache, folding +local ids into its live fetch, and synthesizing stub metadata for disk-only ("BYOM") models. The only +new local concept is the **`kLocal`** tag on those synthesized orphan stubs (see *What "local" means* +below), a short-term marker removed when BYOM lands. The **Private** and **BYOM local** catalogs are +deferred; the source/store split keeps the store source-agnostic so each lands later as a new source, +not a redesign. + +Split the two responsibilities that `BaseModelCatalog` currently couples: + +- **Fetch** moves into a new `IModelSource` interface (one per catalog type). Sources are + **pure fetchers that return `ModelInfo`** — they do not create `Model` instances. +- **Store / query / index / create** lives in a single `ModelCatalog : ICatalog` that owns the + `ModelFactory` and all `Model` instances, merges across sources, keeps shadow duplicates, and + serves a filtered (preferred-only) public API view. + +On a duplicate (same `model_id` across catalog sources) we keep every copy internally as **shadow +variants**; the **public API view** surfaces only the **preferred** one. Preference is by catalog +source: `local > private > public`. Retaining the shadows enables **fallback** — e.g. unregistering +a BYOM local model that shadows a cloud id re-selects the surviving cloud copy. (Genuine cross-source +duplicates only arise once Private or BYOM lands — see *What "local" means* below — so this machinery +is foundational in the initial scope.) + +## What "local" means here + +`ScanLocalModels` (the existing cache scanner, `catalog/local_model_scanner.{h,cc}`) returns +`model_id → local_path` for every model present in the cache dir. The Azure source folds those ids +into its fetch, and they split into two buckets — **only the second is "local":** + +- **Cached public model — *not* local.** The scanned `model_id` resolves against an online catalog + (public latest, or public by-id for an older version). This is a **Public** model with local state + attached: `catalog_source` stays `kPublic`; the fetch marks it cached and sets `local_path`. It + stays a single leaf, not a duplicate. +- **Local stub (`kLocal`) — short-term only.** The scanned `model_id` matches **nothing** in any + online catalog. The Azure catalog synthesizes thin **stub metadata** (`MakeByomModelInfo` → + `AddLocalModels`) so the id is a valid catalog entry, which we tag **`kLocal`**. These + orphan stubs are the *only* genuinely-local entries today. + +The dedicated **BYOM local catalog** (future follow-up) **replaces this stub-based `kLocal` +support** with first-class local models; the short-term stub path exists only until then. No separate +`LocalModelSource` is introduced now — it would add a merge/ownership split with no behavioral gain +while there are no genuine shadows. + +Consequence for the initial scope: public ids are unique and `kLocal` stubs are orphans (disjoint +from public by construction), so **the single Public source produces no genuine cross-source +duplicates**. The shadow-variant / source-preference / `UniqueVariants` machinery is therefore +foundational — first exercised when Private or BYOM introduces a second source that can serve the same +`model_id`. + +## Why this shape + +- **Source/store split; sources return `ModelInfo`.** The store owns the `ModelFactory`, so sources + stay pure fetchers and never touch `DownloadManager` / `ModelLoadManager`. +- **Duplicates are shadow variants, filtered by the public view.** There is no internal + "no duplicates" rule — the user drives selection, so the visible `flModelList` is just a filtered + view, and same-`model_id` copies from different sources may carry different metadata. + +Two behavior changes fall out of this: + +1. **Container `variants_` may hold same-`model_id` shadows** (today they're distinct). Filtering + moves to the visible view; the internal **`id_index`** (the `model_id → Model*` lookup that backs + by-id queries, built by `RebuildIndex`) resolves each `model_id` to its **preferred** leaf via + source-aware ordering. +2. **`Model*` stays stable for cloud models** (append-only; `RemoveFromCache` only un-caches). The + one exception is a **BYOM local `Unregister`** (future) — an explicit, user-initiated removal. + +## Confirmed decisions + +| # | Topic | Decision | Rationale | +|---|---|---|---| +| D1 | Architecture | **Source/store split.** One owning store; sources are fetch-only. | Preserves `Model*` stability and single ownership; reuses existing index/refresh. | +| D2 | Sources return | **`ModelInfo`, not `Model`.** The store owns the `ModelFactory`. | Keeps sources free of `DownloadManager` / `ModelLoadManager` coupling. | +| D3 | Catalog source | **Explicit `CatalogSource` enum field on `ModelInfo`**, not a property-bag entry, and **not** an overload of `model_provider`. | It is correctness-critical (drives dedup/preference), sits on the compare hot path, and is a small closed set. `model_provider` describes *who publishes*; catalog source describes *which catalog served* it. | +| D4 | Duplicate storage | **Shadow variants** inside the alias container; the public API view filters to the preferred copy. | Internal duplicates are harmless (the user controls model selection); the visible list is a filterable `flModelList` view. Keeps each source's full metadata for fallback. | +| D5 | Preference | `local > private > public`, applied by **source-aware variant ordering** so both by-`model_id` lookup and the visible view resolve to the preferred copy. | Small, closed tiebreak; applies only when two sources serve the same `model_id` (the unique model identifier). | +| D6 | Cached-state / non-latest metadata | The local scan attaches `local_path` + cached state to the matching catalog entry. For cached **non-latest** versions absent from the latest cloud fetch, resolve full metadata via the online source's `FetchModelsByIds`. | A downloaded cloud model is not a duplicate — it is the cloud entry with local state attached. Matches current behavior. | +| D7 | Private catalog | A **future follow-up**; an additional online catalog source. Its shape, auth, and fetch implementation are **deferred** — the source/store split leaves room for it without a redesign. | Out of initial scope; captured only as a placeholder. | +| D8 | Local models | **No separate source now.** The Azure source keeps today's flow — scan the cache (`ScanLocalModels`), fetch live metadata resolving cached ids by-id (`FetchAllModelInfosWithCachedModels`), then `AddLocalModels` attaches `local_path` and synthesizes stubs (`MakeByomModelInfo`) for disk-only models. The only change: **tag those synthesized stubs `kLocal`**. The dedicated **BYOM local catalog** replaces this stub path later. | Least risk; reuses working code. A separate `LocalModelSource` adds a merge/ownership split with no gain while there are no shadows. | +| D9 | Removal / lifecycle | **Cloud (`kPublic`)** model: `RemoveFromCache` deletes the local dir but keeps the `Model` (re-downloadable). **BYOM local** model *(future)*: `Unregister` removes the `Model`; `RemoveFromCache` is invalid for it. Short-term `kLocal` orphan stubs have no explicit lifecycle API — they drop out on the next refresh when their cache dir is gone. | Distinct semantics; the future BYOM catalog exposes explicit `Register` / `Unregister`. | +| D10 | Delivery | **Initial scope is the Public (Azure) source only** (with today's inline local-cache resolution + the new `kLocal` tag). The Private catalog and dedicated BYOM local catalog are future follow-ups (each an independent source). | Shrinks the initial diff — the store is source-agnostic, so Private and BYOM are additions, not redesigns. | + +## Current architecture (verified) + +- **`ICatalog`** ([src/catalog.h](../src/catalog.h)) is the query surface; `flCatalog` / `flManager` + wrap `fl::ICatalog&`, so any implementation keeps the C ABI and consumers unchanged. +- **`BaseModelCatalog`** ([base_model_catalog.h](../src/catalog/base_model_catalog.h)) owns `models_` + (stable `unique_ptr`) with pure-virtual `FetchModels` / `FetchModelVersions` / `FetchModelsByIds`. + `PopulateModels` groups leaves by alias; `IntegrateVariants` dedups by `model_id`; `RebuildIndex` + builds `id_index` / `alias_index` / `name_index` (first-wins). +- **`AzureModelCatalog`** is the only subclass. `FetchModels` scans the cache + (`ScanLocalModels`), then `GetLiveCatalogOrLocalSnapshot` fetches live metadata across the catalog + URLs (each via `FetchAllModelInfosWithCachedModels` — fetch latest + resolve cached non-latest + by-id — deduplicated by `model_id`), falling back to the `CatalogCache` / `foundry.modelinfo.json` + snapshot when every live URL fails. `AddLocalModels` then attaches `local_path` to matched infos and + synthesizes stub metadata (`MakeByomModelInfo`) for disk-only models, building leaves via + `model_factory_`. A `CreateCatalogClient` virtual seam exists for test injection. +- **`GetCachedModels`** ([base_model_catalog.cc](../src/catalog/base_model_catalog.cc)) now iterates + each container's `Variants()` and returns every cached leaf (not the alias container). +- **`Model`** ([src/model.h](../src/model.h)) is a leaf or a container owning `variants_` + + `selected_variant_`. `CompareBestFirst` orders device asc, version desc, created-at desc, + `model_id` asc. `RemoveFromCache` only un-caches. + +## Target architecture + +``` + ┌──────────────────────────────┐ + public API ───> │ ModelCatalog : ICatalog │ owns ModelFactory, containers, + / C ABI │ - models_ / indices / cache │ indices, caching, refresh; + │ - vector> sources│ to preferred + └──────────────┬───────────────┘ + │ composes (fetch-only; return ModelInfo) + ┌─────────────────────┼─────────────────────┐ + ▼ ▼ ▼ + ┌────────────────────────────┐ ┌───────────────────┐ ┌───────────────────┐ + │ AzureModelSource (kPublic) │ │ Private source │ │ BYOM Local source │ + │ + inline local-cache │ │ (kPrivate)*future*│ │ (kLocal) *future* │ + │ resolve: kPublic cached │ └───────────────────┘ └───────────────────┘ + │ / kLocal stub │ + └────────────────────────────┘ + IModelSource: FetchModels / FetchModelsByIds / FetchModelVersions → ModelInfo + (The future BYOM Local source replaces AzureModelSource's short-term inline kLocal stubs.) +``` + +- The catalog **has** sources; it **is not** a source. +- Each source stamps `info.catalog_source` on every `ModelInfo` it produces. Short-term the Azure + source stamps `kPublic` on catalog entries (including cached ones with `local_path` attached) and + `kLocal` on the orphan stubs it synthesizes from the local cache scan. +- The store creates a leaf via its owned `ModelFactory` for every `ModelInfo` (including + same-`model_id` shadows), groups them by alias into containers, and orders variants source-aware + so the preferred copy wins in `id_index` and the visible view. + +## Design + +### New / changed types + +- **`CatalogSource { kPublic = 0, kPrivate = 1, kLocal = 2 }`** + `CatalogSourcePriority(src)` + (local ranks most-preferred). `kPublic = 0` so zero-initialized / legacy `ModelInfo` decodes as + public. Preference is expressed by the priority helper, independent of the enum's value order. + The priority helper feeds `CompareBestFirst` as its **final tiebreak** (see below), so it only ever + reorders genuine duplicates and never perturbs the existing device/version/created-at/`model_id` + ordering of non-duplicates. +- **`ModelInfo::catalog_source`** — new field, default `kPublic`; round-tripped in + `ModelInfoFromJson` / `ModelInfoToJson` so it survives the on-disk cache (absent → `kPublic`). +- **`IModelSource`** (new, `src/catalog/model_source.h`), fetch-only, returns `ModelInfo`: + - `CatalogSource Source() const` + - `std::string Name() const` + - `std::vector FetchModels() const` (latest, stamped with `Source()`) + - `std::vector FetchModelsByIds(const std::vector& ids) const` (default `{}`) + - `std::vector FetchModelVersions(const std::string& alias, const std::string& name = "") const` (default `{}`) +- **`AzureModelSource : IModelSource`** — the fetch guts moved out of `AzureModelCatalog`, serving + the **Public** catalog. Parameterized by URLs + filter, region, and fallback; reuses + `MakeCatalogClient` / `AzureCatalogClient` and the `CreateCatalogClient` test seam. + **Short-term it also owns local-cache resolution** (today's flow): `ScanLocalModels` → + `GetLiveCatalogOrLocalSnapshot` (live fetch, else `CatalogCache` snapshot fallback) → + `AddLocalModels`, stamping `kPublic` on catalog entries and **`kLocal`** on the synthesized + (`MakeByomModelInfo`) orphan stubs, and conveying each cached entry's `local_path` so the store + marks it cached. This local handling moves to the BYOM source later. (A future Private source is a + separate follow-up; its shape is TBD.) +- **`ModelCatalog : ICatalog`** — evolves `BaseModelCatalog`. Holds + `std::vector> sources_`, the `ModelFactory`, and the existing + store/indices/refresh. Containers may hold same-`model_id` shadow variants; variant ordering is + **source-aware** (preferred first) so `id_index` first-wins and the visible view resolve to the + preferred copy. Adds a local-BYO `Unregister` path that removes a variant (see Removal & fallback). + +### Variant ordering, selection & visibility (shadow variants) + +Shadow variants (same `model_id` from different catalog sources) live inside the alias container's +`variants_` alongside genuine distinct variants. Three mechanisms keep them internally complete while +the public surface stays de-duplicated: + +- **Ordering — `CompareBestFirst` gains a final tiebreak.** The comparator keeps its existing keys + (device priority asc, version desc, created-at desc, `model_id` asc) and appends + `CatalogSourcePriority(catalog_source)` **asc** as the *last* key. Genuine duplicates match on all + four prior keys, so the source key alone decides their relative order (`local > private > public`); + non-duplicates differ earlier and are unaffected. `AddVariant`'s `upper_bound` insert therefore + places every shadow in preferred-first order regardless of source insertion order. + +- **Internal enumeration — `Variants()` is unchanged and all-inclusive.** It returns every leaf, + including shadows, and remains the accessor used by internal machinery (`RebuildIndex`, + `GetCachedModels` / `GetLoadedModels`, merge/integration). Because `variants_` is preferred-first, + `RebuildIndex`'s first-wins `id_index[model_id]` resolves to the **preferred** leaf automatically. + +- **Public enumeration — new `Model::UniqueVariants()`.** Returns `std::vector` filtered to + one leaf per `model_id`: it walks `variants_` in its existing best-first order (under a single lock) + and keeps the first occurrence of each `model_id` (which, per the tiebreak above, is the + preferred-source copy), skipping later shadows. This is the accessor **both public surfaces** use: + the C API `Model_GetVariantsImpl` (`c_api.cc`) and the REST `GET /v1/models` + `OpenAIListModelsHandler` (`service/models_handlers.cc`) switch from `Variants()` to + `UniqueVariants()`, so neither emits duplicate `model_id`s. The visible `flModelList` / model list is + thus a filtered projection; internal storage keeps the full shadow set for fallback. + + *Why a method, not inline filtering:* it has **two** public callers, so centralizing keeps the + shadow-dedup policy next to the ordering rules rather than duplicated across handlers. It is also + **allocation-neutral** — these sites already call `Variants()` (a heap snapshot vector) and copy + into their output; `UniqueVariants()` returns the same single snapshot under one lock. (A + zero-intermediate callback enumerator was rejected: catalog access is explicitly never + performance-critical.) Internal callers — `RebuildIndex`, `IntegrateVariants`, `GetCachedModels` / + `GetLoadedModels` — keep using the all-inclusive `Variants()`. + +**Default variant selection (`SelectDefaultVariant`) — cached wins.** The existing rule stands: +select the first **cached** variant in best-first order, else `variants_.front()`. Precedence is +therefore *cached-first, then source-preference among equals*: + +- Any **local** model (BYO or a downloaded copy) is locally available, so its leaf is constructed with + its **cached flag set by default**. A local shadow is thus cached and, being highest source + priority, is both first-in-order and cached → selected. +- If a lower-priority source's copy is cached but the preferred-source copy is not (e.g. cached + `public` vs. uncached `private`), the **cached** copy is selected — cached beats source preference, + by design. + +Note a deliberate divergence for shadowed `model_id`s: `id_index[model_id]` (used by +`GetModelVariant`) resolves to the *preferred-source* leaf via first-wins ordering, while the +container's *default selection* may be a *cached* lower-priority leaf. These answer different +questions ("give me the preferred copy of this id" vs. "what does this container act on by default") +and are intentionally allowed to differ. + +### Merge algorithm (`ModelCatalog::Populate`) + +`Populate` keeps its current shape — cache-only mode, fetch, leaf-build via `ModelFactory`, group by +alias (`PopulateModels`), and `RebuildIndex`. The **one change**: gather `ModelInfo` from all sources +(each stamped by `Source()`) and allow same-`model_id` **shadows** — the dedup key in +`IntegrateVariants` becomes `(model_id, catalog_source)` and variant ordering is source-aware +(`local > private > public`) so `id_index` first-wins and the visible view resolve to the preferred +copy. In the initial single-source scope no shadows arise (see *What "local" means*), so this is +dormant foundation. + +### Removal & fallback + +Removal is **foundation, first exercised by BYOM (future)** — the initial single-source scope has no +shadows to fall back to. Semantics (per D9): **cloud (`kPublic`)** uses `RemoveFromCache` (un-caches, +keeps the re-downloadable `Model`, removes no variant — unchanged); short-term **`kLocal`** stubs have +no removal API and drop out on refresh once their cache dir is gone; **BYOM local (future)** adds +`Unregister`, which removes the variant and re-selects the surviving cloud shadow (if any). The path, +specified now: + +**`Model::RemoveVariant(const Model& variant)`** — under `state_mutex_`: find the matching +`unique_ptr` by address (throw `FOUNDRY_LOCAL_ERROR_INTERNAL` if absent); **erase and compact** (no +`nullptr` holes — every `variants_` walker assumes dense, non-null entries; erase preserves best-first +order); if the removed leaf was `selected_variant_`, re-run `SelectDefaultVariant` (cached-first, then +preferred source), or leave it null if `variants_` is now empty. + +**`ModelCatalog::Unregister(model_id)`** — under the catalog mutex: locate the container via +`alias_index`, call `RemoveVariant`, erase the container from `models_` if it became empty, then +`RebuildIndex`. + +**Invariant & concurrency.** This is the one place the historically **append-only** `models_` / +`variants_` invariant is relaxed. Removal is a rare, explicit, user-initiated admin op, so — rather +than adding tombstones or generational handles — we accept: it **invalidates outstanding +`flModelList` / `Model*`** obtained before the call (the erased leaf is freed; survivors keep their +addresses because `variants_` holds `unique_ptr`); it is **not safe to call concurrently** with +enumeration or model ops on the affected alias (quiesce first); normal refresh stays append-only and +concurrency-safe. Header "never removed" promises on `base_model_catalog.h` / `model.h` are updated to +carve out this exception. + +### Wiring + +- **`Manager::Create`** ([src/manager.cc](../src/manager.cc)): build the source list `[Public]` + (the `AzureModelSource`, which also does the inline local-cache resolution), construct the + `ModelFactory`, and construct one `ModelCatalog`. `catalog_` stays `std::unique_ptr`. The + list is source-agnostic, so future Private / BYOM sources slot in without store changes. + +## Delivery phases + +Phase 0–3 are the **initial scope: the Public (Azure) source** (with inline local-cache resolution + +the `kLocal` tag). The **Private catalog** and the dedicated **BYOM local catalog** are **future +follow-ups** — each an independent source that needs no redesign of the store, sources, or public API. + +### Phase 0 — Types & metadata +1. Add `CatalogSource` enum + `CatalogSourcePriority` helper. +2. Add `CatalogSource catalog_source` to `ModelInfo`; round-trip in + `ModelInfoFromJson` / `ModelInfoToJson`. + +### Phase 1 — Source abstraction *(parallel after Phase 0)* +3. New `IModelSource` interface (returns `ModelInfo`). +4. `AzureModelSource` (fetch guts from `AzureModelCatalog`; serves Public and retains the inline + local-cache resolution — `ScanLocalModels` + `GetLiveCatalogOrLocalSnapshot` + `AddLocalModels`, + incl. the snapshot fallback and `CreateCatalogClient` seam). +5. Tag the synthesized orphan stubs `kLocal` in `MakeByomModelInfo` (`azure_model_catalog.cc`) — the + only local-specific change. + +### Phase 2 — Aggregating store *(depends on Phase 1)* +6. `ModelCatalog : ICatalog` — owns `ModelFactory`, `sources_`; reuse group/index/refresh with + source-aware variant ordering, `UniqueVariants()`-backed preferred-only public view, and the + `Unregister` / `RemoveVariant` removal path (foundation; dormant with one source). +7. Implement the merge / leaf-build / shadow-duplicate algorithm above; preserve cache-only mode + and `CatalogCache` save. + +### Phase 3 — Wiring *(depends on Phase 2)* +8. `Manager::Create` builds the `[Public]` source, constructs the factory and `ModelCatalog`. + +### Phase 4 — Tests *(parallel with Phases 2–3)* +9. `FakeModelSource` helper (replaces the `TestCatalog` `FetchModels` override; returns `ModelInfo`). +10. `model_catalog_test.cc`: **local classification** (scanned id matching public → `kPublic` cached + single leaf; orphan id → `kLocal` stub) / preference (`local > private > public`) / shadow-variant + visibility / preferred-only visible view (`UniqueVariants` de-dup) / `SelectDefaultVariant` + precedence (cached-beats-source; local-cached-by-default) / cached-state attachment / + cached-non-latest cloud resolution / fallback-on-unregister via `FakeModelSource` shadows + (`RemoveVariant` compaction + empty-container removal + surviving-shadow re-selection) / + `catalog_source` round-trips through the cache JSON / union-of-aliases across sources. +11. Migrate the Azure catalog tests (`azure_catalog_test.cc` + `azure_model_catalog_test.cc`, incl. the + snapshot-fallback / BYOM-synthesis / dedup cases) → `azure_model_source_test.cc` (fetch guts + unchanged). Keep `catalog_cache_test`, `model_sorting_test`, and `sdk_api/catalog_test` (surface + unchanged). + +## Future follow-ups *(out of initial scope; no redesign required)* + +Each is independent and depends only on Phase 3. All slot into the same source/store design. + +### Private catalog +- An additional online catalog source (its own `IModelSource`, stamped `kPrivate`), plus any + `Configuration` and C-API surface it needs. Shape, auth, and fetch implementation are **TBD** and + designed when the follow-up is scheduled. + +### BYOM local catalog +- A dedicated local source (its own **TBD** scan path + first-class local metadata) and a public + `Register` / `Unregister` API. It **replaces** the Azure source's short-term inline local-cache + resolution and `kLocal` stubs; `Unregister` wires to the `ModelCatalog` variant-removal path. + +### Public visibility of duplicate sources +- Once a real scenario needs it, add a public way for a consumer to enumerate all of a model's + duplicate catalog sources (the shadows the visible view hides). Not built now — the visible view + surfaces only the preferred copy, which is sufficient until a concrete need arises. + +## Affected files + +**New** +- `catalog/model_source.h` (`IModelSource`) +- `catalog/azure_model_source.{h,cc}` +- `catalog/model_catalog.{h,cc}` + +**Modify** +- [src/model_info.h](../src/model_info.h) / [src/model_info.cc](../src/model_info.cc) — + `CatalogSource` enum, `CatalogSourcePriority`, `catalog_source` field + JSON round-trip +- [src/model.h](../src/model.h) / [src/model.cc](../src/model.cc) — source-aware **final tiebreak** + in `CompareBestFirst` (orders genuine duplicates preferred-first, non-duplicates unchanged); new + `UniqueVariants()` preferred-only view for the public list (leaving `Variants()` all-inclusive for + internal use); `RemoveVariant` for local-BYO `Unregister`; local leaves constructed cached-by-default +- [src/catalog/azure_model_catalog.cc](../src/catalog/azure_model_catalog.cc) — `MakeByomModelInfo` + (the disk-only stub synthesizer) stamps `catalog_source = kLocal` on the stubs it + produces (the only local-specific change). `FetchAllModelInfosWithCachedModels` + ([catalog_client.cc](../src/catalog/catalog_client.cc)) — which now only fetches latest + resolves + cached ids by-id — is untouched. +- [src/manager.cc](../src/manager.cc) / [src/manager.h](../src/manager.h) — build the `[Public]` + source (~L325), construct the factory + `ModelCatalog` (factory `CreateModel` at ~L545) +- [src/catalog/catalog_cache.h](../src/catalog/catalog_cache.h) / + [src/catalog/catalog_cache.cc](../src/catalog/catalog_cache.cc) — round-trips `catalog_source` + via `ModelInfo` JSON +- [include/foundry_local/foundry_local_c.h](../include/foundry_local/foundry_local_c.h) / + [src/c_api.cc](../src/c_api.cc) — read-only `catalog_source` (int) on `flModelInfo`; + `Model_GetVariantsImpl` switches from `Variants()` to `UniqueVariants()` so the public list is + de-duplicated. Any Private-catalog C-API surface stays **append-only** and is a future follow-up. +- [src/service/models_handlers.cc](../src/service/models_handlers.cc) — `OpenAIListModelsHandler` + (`GET /v1/models`) switches from `Variants()` to `UniqueVariants()` so it never emits duplicate + `model_id`s across catalog sources +- [CMakeLists.txt](../CMakeLists.txt) — add/remove sources + +**Remove / absorb** +- `catalog/base_model_catalog.{h,cc}` → `catalog/model_catalog.{h,cc}` +- `catalog/azure_model_catalog.{h,cc}` → `catalog/azure_model_source.{h,cc}` (keeps the inline + local-cache resolution: `ScanLocalModels` + `GetLiveCatalogOrLocalSnapshot` + `AddLocalModels` / + `MakeByomModelInfo`, incl. the snapshot fallback and `CreateCatalogClient` seam) +- `FetchAllModelInfosWithCachedModels` ([catalog/catalog_client.cc](../src/catalog/catalog_client.cc)) + **stays** — reused by `AzureModelSource`; unchanged (stub synthesis lives in `MakeByomModelInfo`). + `ICatalogClient` in `catalog_client.h` stays (used by `AzureModelSource`). + +## Verification + +1. `python sdk_v2/cpp/build.py --build --config Debug` +2. `foundry_local_tests.exe --gtest_filter="ModelCatalog*:*Source*:*Catalog*"` (fast; no model load) +3. Optional live: `sdk_integration_tests` `catalog_live` (real Azure fetch) +4. C# tests auto-load the fresh native via `foundry_local.native.cfg` — keep C ABI additions + **append-only** so struct layout stays stable. + +## Scope boundaries + +- **Included (initial scope)**: the Public (Azure) source with inline local-cache resolution (cached + `kPublic` entries + short-term `kLocal` orphan stubs, per *What "local" means*); the + shadow / preference / `UniqueVariants` machinery and the `Unregister` / `RemoveVariant` path as + dormant foundation. +- **Excluded (future follow-ups)**: the **Private** catalog and the dedicated **BYOM local** catalog + (which replaces the interim `kLocal` stubs and adds `Register` / `Unregister`). Both are new + sources, not redesigns; details TBD. From ff31dfd4219a1619d1a1f0f9a7899cd6f280ce28 Mon Sep 17 00:00:00 2001 From: Scott McKay Date: Sat, 15 Aug 2026 18:19:48 +1000 Subject: [PATCH 2/4] Initial implementation. --- sdk_v2/cpp/CMakeLists.txt | 4 +- .../include/foundry_local/foundry_local_c.h | 4 + sdk_v2/cpp/src/c_api.cc | 9 +- sdk_v2/cpp/src/catalog/azure_model_catalog.h | 76 --- ...model_catalog.cc => azure_model_source.cc} | 114 ++-- sdk_v2/cpp/src/catalog/azure_model_source.h | 88 ++++ sdk_v2/cpp/src/catalog/base_model_catalog.h | 133 ----- ...base_model_catalog.cc => model_catalog.cc} | 192 ++++++- sdk_v2/cpp/src/catalog/model_catalog.h | 140 +++++ sdk_v2/cpp/src/catalog/model_source.h | 50 ++ sdk_v2/cpp/src/manager.cc | 26 +- sdk_v2/cpp/src/manager.h | 2 +- sdk_v2/cpp/src/model.cc | 110 +++- sdk_v2/cpp/src/model.h | 40 +- sdk_v2/cpp/src/model_info.cc | 38 ++ sdk_v2/cpp/src/model_info.h | 42 ++ sdk_v2/cpp/src/service/models_handlers.cc | 3 +- sdk_v2/cpp/test/CMakeLists.txt | 4 +- .../internal_api/audio/audio_session_test.cc | 6 +- ...log_test.cc => azure_model_source_test.cc} | 165 +++--- .../internal_api/base_model_catalog_test.cc | 448 ---------------- .../internal_api/chat/chat_session_test.cc | 2 +- .../test/internal_api/model_catalog_test.cc | 498 ++++++++++++++++++ .../test/internal_api/model_io_info_test.cc | 2 +- .../test/internal_api/model_sorting_test.cc | 73 ++- .../test/internal_api/session_manager_test.cc | 6 +- sdk_v2/cpp/test/internal_api/test_helpers.h | 2 +- .../cpp/test/internal_api/web_service_test.cc | 9 +- 28 files changed, 1390 insertions(+), 896 deletions(-) delete mode 100644 sdk_v2/cpp/src/catalog/azure_model_catalog.h rename sdk_v2/cpp/src/catalog/{azure_model_catalog.cc => azure_model_source.cc} (62%) create mode 100644 sdk_v2/cpp/src/catalog/azure_model_source.h delete mode 100644 sdk_v2/cpp/src/catalog/base_model_catalog.h rename sdk_v2/cpp/src/catalog/{base_model_catalog.cc => model_catalog.cc} (68%) create mode 100644 sdk_v2/cpp/src/catalog/model_catalog.h create mode 100644 sdk_v2/cpp/src/catalog/model_source.h rename sdk_v2/cpp/test/internal_api/{azure_model_catalog_test.cc => azure_model_source_test.cc} (64%) delete mode 100644 sdk_v2/cpp/test/internal_api/base_model_catalog_test.cc create mode 100644 sdk_v2/cpp/test/internal_api/model_catalog_test.cc diff --git a/sdk_v2/cpp/CMakeLists.txt b/sdk_v2/cpp/CMakeLists.txt index fc5c189e4..40ae5bee1 100644 --- a/sdk_v2/cpp/CMakeLists.txt +++ b/sdk_v2/cpp/CMakeLists.txt @@ -199,8 +199,8 @@ set(FOUNDRY_LOCAL_SOURCES src/items/image_item.cc src/items/message_item.cc src/items/speech_segment_item.cc - src/catalog/base_model_catalog.cc - src/catalog/azure_model_catalog.cc + src/catalog/model_catalog.cc + src/catalog/azure_model_source.cc src/catalog/azure_catalog_client.cc src/catalog/azure_catalog_models.cc src/catalog/catalog_cache.cc diff --git a/sdk_v2/cpp/include/foundry_local/foundry_local_c.h b/sdk_v2/cpp/include/foundry_local/foundry_local_c.h index fa1f63738..f0360d0c7 100644 --- a/sdk_v2/cpp/include/foundry_local/foundry_local_c.h +++ b/sdk_v2/cpp/include/foundry_local/foundry_local_c.h @@ -1040,6 +1040,10 @@ struct flModelApi { /// Use FL_MODEL_PROP_* constants for well-known keys, or any arbitrary string key. int64_t FL_API_T(Info_GetIntProperty, _In_ const flModelInfo* info, _In_ const char* key, int64_t default_value); + /// Which catalog source served this model: 0 = public (Azure), 1 = private, 2 = local. + /// Read-only. Defaults to 0 (public) for models without an explicit source. + int FL_API_T(Info_GetCatalogSource, _In_ const flModelInfo* info); + // End V1 }; diff --git a/sdk_v2/cpp/src/c_api.cc b/sdk_v2/cpp/src/c_api.cc index 6e89e534f..c87bce99a 100644 --- a/sdk_v2/cpp/src/c_api.cc +++ b/sdk_v2/cpp/src/c_api.cc @@ -857,7 +857,9 @@ FL_API_STATUS_IMPL(Model_GetVariantsImpl, const flModel* model, flModelList** ou return MakeStatus(FOUNDRY_LOCAL_ERROR_INVALID_ARGUMENT, "null argument"); } - auto variants = AsImpl(model)->Variants(); + // Public list is de-duplicated to one leaf per model_id (preferred-source copy); internal + // storage keeps all shadow variants. See MultiCatalogSupportPlan.md. + auto variants = AsImpl(model)->UniqueVariants(); auto list = std::make_unique(); list->items.reserve(variants.size()); @@ -969,6 +971,10 @@ static int64_t FL_API_CALL Info_GetIntPropertyImpl(const flModelInfo* info, return AsImpl(info)->GetPropertyWithDefault(key, default_value); } +static int FL_API_CALL Info_GetCatalogSourceImpl(const flModelInfo* info) FL_NO_EXCEPTION { + return info ? static_cast(AsImpl(info)->catalog_source) : 0; +} + static const flModelApi g_model_api = { Model_GetInfoImpl, Model_GetInputOutputInfoImpl, @@ -993,6 +999,7 @@ static const flModelApi g_model_api = { Info_GetModelSettingsImpl, Info_GetStringPropertyImpl, Info_GetIntPropertyImpl, + Info_GetCatalogSourceImpl, }; // ======================================================================== diff --git a/sdk_v2/cpp/src/catalog/azure_model_catalog.h b/sdk_v2/cpp/src/catalog/azure_model_catalog.h deleted file mode 100644 index b7bb3bc6e..000000000 --- a/sdk_v2/cpp/src/catalog/azure_model_catalog.h +++ /dev/null @@ -1,76 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. -#pragma once - -#include "catalog/base_model_catalog.h" -#include "ep_detection/ep_detector.h" -#include "logger.h" - -#include -#include -#include -#include -#include -#include -#include - -namespace fl { - -class ICatalogClient; - -/// Azure-specific catalog. Fetches from Azure Foundry catalog API, -/// scans local cache, merges results. -/// Maps to C# AzureModelCatalog. -class AzureModelCatalog : public BaseModelCatalog { - public: - using ModelFactory = std::function; - - AzureModelCatalog(std::vector>> catalog_urls, - std::string cache_dir, - ModelFactory model_factory, - const IEpDetector& ep_detector, - ILogger& logger, - bool cache_only = false, - std::string catalog_region = "", - bool disable_region_fallback = false); - ~AzureModelCatalog() override; - - protected: - std::vector FetchModels() const override; - std::vector FetchModelVersions(const std::string& model_alias, - const std::string& model_name = "") const override; - std::vector FetchModelsByIds(const std::vector& model_ids) const override; - virtual std::unique_ptr CreateCatalogClient(const std::string& url, - const std::string& filter) const; - - private: - using LocalModels = std::map; - - enum class CatalogSource { - kLive, - kSnapshot, - }; - - struct CatalogResult { - std::vector model_infos; - CatalogSource source; - }; - - static constexpr const char* kDefaultCatalogUrl = "https://ai.azure.com/api/centralus/ux/v1.0"; - static constexpr const char* kDefaultCatalogFilter = "''"; - - CatalogResult GetLiveCatalogOrLocalSnapshot(const std::vector& cached_model_ids) const; - std::vector AddLocalModels(std::vector& model_infos, const LocalModels& local_models) const; - - std::vector>> catalog_urls_; - std::string cache_dir_; - ModelFactory model_factory_; - const IEpDetector& ep_detector_; - ILogger& logger_; - bool cache_only_; - // Configured Azure region: empty/"auto" → auto-detect, explicit → hard override. - std::string catalog_region_; - bool disable_region_fallback_; -}; - -} // namespace fl diff --git a/sdk_v2/cpp/src/catalog/azure_model_catalog.cc b/sdk_v2/cpp/src/catalog/azure_model_source.cc similarity index 62% rename from sdk_v2/cpp/src/catalog/azure_model_catalog.cc rename to sdk_v2/cpp/src/catalog/azure_model_source.cc index 63776436c..ab249a351 100644 --- a/sdk_v2/cpp/src/catalog/azure_model_catalog.cc +++ b/sdk_v2/cpp/src/catalog/azure_model_source.cc @@ -1,10 +1,9 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -#include "catalog/azure_model_catalog.h" +#include "catalog/azure_model_source.h" #include "catalog/catalog_cache.h" #include "catalog/catalog_client.h" #include "catalog/local_model_scanner.h" -#include "model.h" #include "model_info.h" #include "utils.h" @@ -29,6 +28,9 @@ ModelInfo MakeByomModelInfo(const std::string& model_id) { info.alias = name; info.uri = "local://" + name; info.version = version; + // Short-term marker: a disk-only model that matches nothing in any online catalog. + // Replaced by first-class local models when the dedicated BYOM catalog lands. + info.catalog_source = CatalogSource::kLocal; info.string_properties[FOUNDRY_LOCAL_MODEL_PROP_MODEL_PROVIDER_STR] = "Local"; info.string_properties[FOUNDRY_LOCAL_MODEL_PROP_MODEL_TYPE_STR] = "ONNX"; return info; @@ -50,18 +52,16 @@ std::vector DeduplicateByModelId(std::vector model_infos) } // namespace -AzureModelCatalog::AzureModelCatalog(std::vector>> catalog_urls, - std::string cache_dir, - ModelFactory model_factory, - const IEpDetector& ep_detector, - ILogger& logger, - bool cache_only, - std::string catalog_region, - bool disable_region_fallback) - : BaseModelCatalog(catalog_urls.empty() ? kDefaultCatalogUrl : catalog_urls.front().first, logger), +AzureModelSource::AzureModelSource(std::vector>> catalog_urls, + std::string cache_dir, + const IEpDetector& ep_detector, + ILogger& logger, + bool cache_only, + std::string catalog_region, + bool disable_region_fallback) + : name_(catalog_urls.empty() ? kDefaultCatalogUrl : catalog_urls.front().first), catalog_urls_(std::move(catalog_urls)), cache_dir_(std::move(cache_dir)), - model_factory_(std::move(model_factory)), ep_detector_(ep_detector), logger_(logger), cache_only_(cache_only), @@ -72,18 +72,17 @@ AzureModelCatalog::AzureModelCatalog(std::vector AzureModelCatalog::CreateCatalogClient(const std::string& url, - const std::string& filter) const { +std::unique_ptr AzureModelSource::CreateCatalogClient(const std::string& url, + const std::string& filter) const { return MakeCatalogClient(url, filter, ep_detector_, logger_, cache_dir_, catalog_region_, disable_region_fallback_); } -AzureModelCatalog::CatalogResult AzureModelCatalog::GetLiveCatalogOrLocalSnapshot( +AzureModelSource::CatalogResult AzureModelSource::GetLiveCatalogOrLocalSnapshot( const std::vector& cached_model_ids) const { if (!cache_only_) { std::vector live_model_infos; @@ -107,7 +106,7 @@ AzureModelCatalog::CatalogResult AzureModelCatalog::GetLiveCatalogOrLocalSnapsho if (any_url_succeeded) { return { .model_infos = DeduplicateByModelId(std::move(live_model_infos)), - .source = CatalogSource::kLive, + .origin = FetchOrigin::kLive, }; } } @@ -118,38 +117,39 @@ AzureModelCatalog::CatalogResult AzureModelCatalog::GetLiveCatalogOrLocalSnapsho return { .model_infos = cached ? DeduplicateByModelId(std::move(*cached)) : std::vector{}, - .source = CatalogSource::kSnapshot, + .origin = FetchOrigin::kSnapshot, }; } -std::vector AzureModelCatalog::AddLocalModels(std::vector& model_infos, - const LocalModels& local_models) const { - std::vector models; - models.reserve(model_infos.size() + local_models.size()); - +void AzureModelSource::AddLocalModels(std::vector& model_infos, + const LocalModels& local_models) const { std::unordered_set model_ids; model_ids.reserve(model_infos.size() + local_models.size()); - for (const auto& info : model_infos) { + + // Attach local paths to matched catalog entries. Their catalog_source is left as-is + // (kPublic by default for live/legacy infos, or the round-tripped value from a snapshot). + for (auto& info : model_infos) { model_ids.insert(info.model_id); 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))); + if (local_model != local_models.end()) { + info.local_path = local_model->second; + } } + // Synthesize kLocal stubs for disk-only models that match nothing in the catalog. for (const auto& [model_id, local_path] : local_models) { if (!model_ids.insert(model_id).second) { continue; } - model_infos.push_back(MakeByomModelInfo(model_id)); - models.push_back(model_factory_(ModelInfo(model_infos.back()), local_path)); + auto stub = MakeByomModelInfo(model_id); + stub.local_path = local_path; + model_infos.push_back(std::move(stub)); } - - return models; } -std::vector AzureModelCatalog::FetchModels() const { +std::vector AzureModelSource::FetchModels() const { logger_.Log(LogLevel::Information, "Getting catalog metadata and locally cached models."); auto local_models = ScanLocalModels(cache_dir_, logger_); @@ -162,26 +162,31 @@ std::vector AzureModelCatalog::FetchModels() const { logger_.Log(LogLevel::Information, fmt::format("Found {} locally cached models.", cached_model_ids.size())); auto catalog_result = GetLiveCatalogOrLocalSnapshot(cached_model_ids); - auto models = AddLocalModels(catalog_result.model_infos, local_models); - logger_.Log(LogLevel::Information, fmt::format("Populated model info for {} models.", models.size())); + // Save the pristine catalog metadata (before local paths / stubs are folded in) so the + // snapshot stays a faithful record of the live catalog. Stubs still get persisted because + // AddLocalModels appended them to the same vector historically — preserve that by saving + // after AddLocalModels. local_path is persisted only when it still exists on disk at save + // time (ModelInfoToJson validates), and a fresh scan overrides it on every fetch. + AddLocalModels(catalog_result.model_infos, local_models); + + logger_.Log(LogLevel::Information, + fmt::format("Populated model info for {} models.", catalog_result.model_infos.size())); - if (catalog_result.source == CatalogSource::kLive && !catalog_result.model_infos.empty()) { + if (catalog_result.origin == FetchOrigin::kLive && !catalog_result.model_infos.empty()) { CatalogCache cache(cache_dir_, logger_); cache.Save(catalog_result.model_infos); } - return models; + return std::move(catalog_result.model_infos); } -std::vector AzureModelCatalog::FetchModelVersions( - const std::string& model_alias, - const std::string& model_name) const { - std::vector out; +std::vector AzureModelSource::FetchModelVersions(const std::string& model_alias, + const std::string& model_name) const { + std::vector out; if (cache_only_) { // In cache-only mode we have no remote source to query for older versions. - logger_.Log(LogLevel::Debug, - "FetchModelVersions skipped: catalog is in cache-only mode."); + logger_.Log(LogLevel::Debug, "FetchModelVersions skipped: catalog is in cache-only mode."); return out; } @@ -192,7 +197,7 @@ std::vector AzureModelCatalog::FetchModelVersions( out.reserve(out.size() + model_infos.size()); for (auto& info : model_infos) { - out.push_back(model_factory_(std::move(info), /*local_path=*/"")); + out.push_back(std::move(info)); } } catch (const std::exception& ex) { logger_.Log(LogLevel::Error, @@ -201,28 +206,26 @@ std::vector AzureModelCatalog::FetchModelVersions( } logger_.Log(LogLevel::Information, - fmt::format("FetchModelVersions('{}') returned {} variant(s).", - model_alias, out.size())); + fmt::format("FetchModelVersions('{}') returned {} variant(s).", model_alias, out.size())); return out; } -std::vector AzureModelCatalog::FetchModelsByIds(const std::vector& model_ids) const { +std::vector AzureModelSource::FetchModelsByIds(const std::vector& model_ids) const { if (model_ids.empty()) { return {}; } if (cache_only_) { - logger_.Log(LogLevel::Debug, - "FetchModelsByIds skipped: catalog is in cache-only mode."); + logger_.Log(LogLevel::Debug, "FetchModelsByIds skipped: catalog is in cache-only mode."); return {}; } auto local_models = ScanLocalModels(cache_dir_, logger_); - std::vector models; - // Track which IDs are still unresolved so we can stop calling further - // endpoints once everything has been found. + std::vector out; + // Track which IDs are still unresolved so we can stop calling further endpoints once + // everything has been found. std::vector remaining(model_ids); for (const auto& [url, filter] : catalog_urls_) { @@ -235,10 +238,9 @@ std::vector AzureModelCatalog::FetchModelsByIds(const std::vectorFetchModelsByIds(remaining); for (auto& info : model_infos) { - std::string local_path; auto it = local_models.find(info.model_id); if (it != local_models.end()) { - local_path = it->second; + info.local_path = it->second; } // Drop this id from the remaining list now that it's resolved. @@ -247,7 +249,7 @@ std::vector AzureModelCatalog::FetchModelsByIds(const std::vector AzureModelCatalog::FetchModelsByIds(const std::vector +#include +#include +#include +#include +#include + +namespace fl { + +class ICatalogClient; + +/// AzureModelSource — the Public (Azure) catalog source. Fetches from the Azure Foundry +/// catalog API and, short-term, also owns local-cache resolution: it scans the cache, +/// folds cached ids into the live fetch, attaches each cached entry's local path, and +/// synthesizes thin stub metadata (tagged `kLocal`) for disk-only ("BYOM") models. +/// +/// Returns `ModelInfo` only — the aggregating ModelCatalog owns the ModelFactory and builds +/// the `Model` leaves. The dedicated BYOM-local source (future) will replace the inline +/// `kLocal` stub handling here. +/// +/// Ported from the former AzureModelCatalog (fetch guts unchanged). +class AzureModelSource : public IModelSource { + public: + AzureModelSource(std::vector>> catalog_urls, + std::string cache_dir, + const IEpDetector& ep_detector, + ILogger& logger, + bool cache_only = false, + std::string catalog_region = "", + bool disable_region_fallback = false); + ~AzureModelSource() override; + + CatalogSource Source() const override { return CatalogSource::kPublic; } + std::string Name() const override { return name_; } + + std::vector FetchModels() const override; + std::vector FetchModelsByIds(const std::vector& model_ids) const override; + std::vector FetchModelVersions(const std::string& model_alias, + const std::string& model_name = "") const override; + + protected: + /// Test seam: construct the live catalog client. Overridden in tests to inject a fake. + virtual std::unique_ptr CreateCatalogClient(const std::string& url, + const std::string& filter) const; + + private: + using LocalModels = std::map; + + enum class FetchOrigin { + kLive, + kSnapshot, + }; + + struct CatalogResult { + std::vector model_infos; + FetchOrigin origin; + }; + + static constexpr const char* kDefaultCatalogUrl = "https://ai.azure.com/api/centralus/ux/v1.0"; + static constexpr const char* kDefaultCatalogFilter = "''"; + + CatalogResult GetLiveCatalogOrLocalSnapshot(const std::vector& cached_model_ids) const; + + /// Attach local paths to matched catalog infos and append synthesized `kLocal` stubs for + /// disk-only models. Mutates `model_infos` in place (appends stubs). + void AddLocalModels(std::vector& model_infos, const LocalModels& local_models) const; + + std::string name_; + std::vector>> catalog_urls_; + std::string cache_dir_; + const IEpDetector& ep_detector_; + ILogger& logger_; + bool cache_only_; + // Configured Azure region: empty/"auto" → auto-detect, explicit → hard override. + std::string catalog_region_; + bool disable_region_fallback_; +}; + +} // namespace fl diff --git a/sdk_v2/cpp/src/catalog/base_model_catalog.h b/sdk_v2/cpp/src/catalog/base_model_catalog.h deleted file mode 100644 index 97411e395..000000000 --- a/sdk_v2/cpp/src/catalog/base_model_catalog.h +++ /dev/null @@ -1,133 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. -#pragma once - -#include "catalog.h" -#include "logger.h" - -#include -#include -#include -#include -#include -#include - -namespace fl { - -/// BaseModelCatalog: holds model collection, indices, and lookup logic. -/// Derived classes implement FetchModels() to provide models from their specific source. -/// The base class owns caching, indexing, and thread-safe refresh. -/// -/// Model ownership: The catalog owns all Model instances via unique_ptr in models_. -/// These pointers are stable for the lifetime of the catalog — external code can hold -/// raw Model* pointers safely. Indices (id_index, alias_index, name_index) are rebuilt -/// on refresh but always point into the stable models_ storage. GetModelVersions uses -/// separate append-only storage (version_query_models_) — its results are query-only -/// and not integrated into the main indices, but all returned pointers remain valid -/// for the catalog's lifetime. -/// -/// Maps to C# BaseModelCatalog. -class BaseModelCatalog : public ICatalog { - public: - BaseModelCatalog(std::string name, ILogger& logger); - ~BaseModelCatalog() override; - - const std::string& GetName() const override { return name_; } - - // ICatalog implementations — query/lookup layer - std::vector ListModels() const override; - Model* GetModel(const std::string& alias) const override; - Model* GetModelVariant(const std::string& model_id) const override; - Model* GetLatestVersion(const Model* model) const override; - std::vector GetCachedModels() const override; - std::vector GetLoadedModels() const override; - std::vector GetModelVersions(const std::string& model_alias, - const std::string& variant_name, - int max_versions = 0) override; - void InvalidateCache() override; - - protected: - /// Derived classes implement this to fetch model variants from their source. - /// Returns the full variant list. Base class handles caching and indexing. - /// Maps to C# FetchModelInfoAsync. - virtual std::vector FetchModels() const = 0; - - /// Derived classes implement this to fetch all versions of a model from the - /// underlying catalog source, bypassing the "latest only" filter. - /// Returns the variants for the given alias. AddVariant inserts them in - /// priority order automatically, matching the model list output ordering. - /// Default implementation returns `{}` (no remote source — local-only catalogs). - /// Maps to C# `BaseModelCatalog.GetModelVersionsAsync` -> derived overrides. - virtual std::vector FetchModelVersions( - const std::string& /*model_alias*/, - const std::string& /*model_name*/ = "") const { - return {}; - } - - /// Derived classes implement this to look up specific model versions by ID - /// from the underlying catalog source (e.g., older versions not in the - /// latest catalog). Empty list if `model_ids` is empty. - /// Default implementation returns `{}`. - /// Maps to C# `BaseModelCatalog.FetchLocalModelsAsync`. - virtual std::vector FetchModelsByIds(const std::vector& /*model_ids*/) const { - return {}; - } - - private: - /// Lookup indices into the stable models_ storage. - /// Rebuilt on refresh. Does not own any Model instances. - struct ModelIndex { - std::unordered_map id_index; // model_id -> Model* (specific variant) - std::unordered_map alias_index; // alias -> Model* (grouped container) - std::unordered_map name_index; // name -> latest version Model* - }; - - /// Stable model storage. unique_ptr ensures addresses never change. - /// Models are only appended, never removed — external Model* pointers remain valid. - mutable std::vector> models_; - - /// Lookup indices, rebuilt on each populate/refresh. - /// Guarded by std::atomic_load/store free functions so readers get a consistent - /// snapshot — the swap after rebuild is atomic, so a concurrent reader never sees - /// a partially-built index. - /// Can't use std::atomic> due to lack of implemention in XCode (macOS) - mutable std::shared_ptr index_; - - /// Atomically grab the current index snapshot. Callers hold the returned shared_ptr - /// for the duration of their lookup, keeping the index alive if a refresh swaps it out. - std::shared_ptr GetIndex() const; - - mutable bool populated_ = false; - mutable std::mutex mutex_; - mutable std::chrono::steady_clock::time_point next_refresh_at_{}; - - static constexpr std::chrono::hours kCacheDuration{4}; - - /// Populate or refresh the catalog (under lock). Groups variants, builds indices. - void PopulateModels(std::vector variants) const; - - /// Merge new variants into the catalog's stable storage. For an - /// existing alias container, appends any variants whose model_id isn't already - /// present. For new aliases, creates a new container. Rebuilds the lookup - /// index when the model set actually changed. - void IntegrateVariants(std::vector variants) const; - - /// Build lookup indices from the current models_ collection. - /// Builds a complete new ModelIndex locally, then atomically swaps it into index_. - void RebuildIndex() const; - - /// Thread-safe access: ensures catalog is populated, refreshes if allowed and stale. - void EnsurePopulated(bool allow_refresh = false) const; - - /// Append-only storage for GetModelVersions query results. Each call appends a new - /// container, so all previously returned Model* pointers remain valid for the catalog's - /// lifetime. These models are intentionally not integrated into the main lookup indices. - /// Each entry is a container Model (created via MakeContainer) whose variants are the - /// individual version results — mirroring the structure used by the main models_ list. - mutable std::vector> version_query_models_; - - std::string name_; - ILogger& logger_; -}; - -} // namespace fl diff --git a/sdk_v2/cpp/src/catalog/base_model_catalog.cc b/sdk_v2/cpp/src/catalog/model_catalog.cc similarity index 68% rename from sdk_v2/cpp/src/catalog/base_model_catalog.cc rename to sdk_v2/cpp/src/catalog/model_catalog.cc index 29365fe54..2a004c58d 100644 --- a/sdk_v2/cpp/src/catalog/base_model_catalog.cc +++ b/sdk_v2/cpp/src/catalog/model_catalog.cc @@ -1,6 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -#include "catalog/base_model_catalog.h" +#include "catalog/model_catalog.h" #include @@ -12,17 +12,114 @@ #include #include #include +#include namespace fl { -BaseModelCatalog::BaseModelCatalog(std::string name, ILogger& logger) - : name_(std::move(name)), logger_(logger) {} -BaseModelCatalog::~BaseModelCatalog() = default; +namespace { -void BaseModelCatalog::PopulateModels(std::vector variants) const { - // Group variants by alias into Model containers. - // Matches C# Catalog.UpdateModels() pattern: - // foreach (modelInfo) { find or create Model by alias, add variant } +/// Dedup key for shadow variants: same model_id from different catalog sources is kept. +std::string VariantKey(const ModelInfo& info) { + return info.model_id + '\x1f' + std::to_string(static_cast(info.catalog_source)); +} + +} // namespace + +ModelCatalog::ModelCatalog(std::string name, + std::vector> sources, + ModelFactory model_factory, + ILogger& logger) + : name_(std::move(name)), + sources_(std::move(sources)), + model_factory_(std::move(model_factory)), + logger_(logger) {} + +ModelCatalog::~ModelCatalog() = default; + +Model ModelCatalog::BuildLeaf(ModelInfo info) const { + // ModelInfo carries local_path when the model is cached locally; the factory reads it to + // mark the constructed leaf cached. + return model_factory_(std::move(info)); +} + +std::vector ModelCatalog::FetchModels() const { + std::vector models; + + for (const auto& source : sources_) { + std::vector infos; + try { + infos = source->FetchModels(); + } catch (const std::exception& ex) { + logger_.Log(LogLevel::Error, + fmt::format("FetchModels: source '{}' failed — {}", source->Name(), ex.what())); + continue; + } catch (...) { + logger_.Log(LogLevel::Error, + fmt::format("FetchModels: source '{}' failed — unknown error", source->Name())); + continue; + } + + models.reserve(models.size() + infos.size()); + for (auto& info : infos) { + models.push_back(BuildLeaf(std::move(info))); + } + } + + return models; +} + +std::vector ModelCatalog::FetchModelVersions(const std::string& model_alias, + const std::string& model_name) const { + std::vector out; + + for (const auto& source : sources_) { + std::vector infos; + try { + infos = source->FetchModelVersions(model_alias, model_name); + } catch (const std::exception& ex) { + logger_.Log(LogLevel::Error, + fmt::format("FetchModelVersions: source '{}' failed — {}", source->Name(), ex.what())); + continue; + } + + out.reserve(out.size() + infos.size()); + for (auto& info : infos) { + out.push_back(BuildLeaf(std::move(info))); + } + } + + return out; +} + +std::vector ModelCatalog::FetchModelsByIds(const std::vector& model_ids) const { + if (model_ids.empty()) { + return {}; + } + + std::vector out; + + for (const auto& source : sources_) { + std::vector infos; + try { + infos = source->FetchModelsByIds(model_ids); + } catch (const std::exception& ex) { + logger_.Log(LogLevel::Error, + fmt::format("FetchModelsByIds: source '{}' failed — {}", source->Name(), ex.what())); + continue; + } + + out.reserve(out.size() + infos.size()); + for (auto& info : infos) { + out.push_back(BuildLeaf(std::move(info))); + } + } + + return out; +} + +void ModelCatalog::PopulateModels(std::vector variants) const { + // Group variants by alias into Model containers. Same-model_id shadows from different + // sources are all added as variants; AddVariant keeps them preferred-source first. std::map alias_to_model; for (auto& v : variants) { @@ -88,7 +185,7 @@ void BaseModelCatalog::PopulateModels(std::vector variants) const { populated_ = true; } -void BaseModelCatalog::IntegrateVariants(std::vector variants) const { +void ModelCatalog::IntegrateVariants(std::vector variants) const { std::lock_guard lock(mutex_); if (variants.empty()) { @@ -102,12 +199,12 @@ void BaseModelCatalog::IntegrateVariants(std::vector variants) const { alias_to_existing[m->Alias()] = m.get(); } - // Track existing model_ids in a single set so the dedup check is O(1) and - // doesn't require walking each container's variants per incoming variant. - std::unordered_set existing_ids; + // Track existing (model_id, catalog_source) keys so a same-model_id shadow from a different + // source is still admitted, while an exact duplicate is skipped. + std::unordered_set existing_keys; for (auto& m : models_) { for (auto* v : m->Variants()) { - existing_ids.insert(v->Info().model_id); + existing_keys.insert(VariantKey(v->Info())); } } @@ -126,11 +223,12 @@ void BaseModelCatalog::IntegrateVariants(std::vector variants) const { continue; } - if (existing_ids.count(info.model_id) > 0) { + auto key = VariantKey(info); + if (existing_keys.count(key) > 0) { continue; } - existing_ids.insert(info.model_id); + existing_keys.insert(std::move(key)); new_by_alias[info.alias].push_back(std::move(v)); } @@ -166,12 +264,14 @@ void BaseModelCatalog::IntegrateVariants(std::vector variants) const { } } -void BaseModelCatalog::RebuildIndex() const { +void ModelCatalog::RebuildIndex() const { auto new_index = std::make_shared(); for (auto& m : models_) { new_index->alias_index[m->Alias()] = m.get(); + // Variants() is preferred-first, so first-wins on id_index resolves each model_id to its + // preferred-source leaf and shadows never overwrite it. for (auto* variant : m->Variants()) { const auto& info = variant->Info(); @@ -198,7 +298,7 @@ void BaseModelCatalog::RebuildIndex() const { #endif } -std::shared_ptr BaseModelCatalog::GetIndex() const { +std::shared_ptr ModelCatalog::GetIndex() const { #if defined(_MSC_VER) #pragma warning(push) #pragma warning(disable : 4996) @@ -209,7 +309,7 @@ std::shared_ptr BaseModelCatalog::GetIndex() #endif } -void BaseModelCatalog::InvalidateCache() { +void ModelCatalog::InvalidateCache() { // Reset the refresh timer so the next query triggers a re-fetch. // This is called after EP registration changes — the catalog needs to // re-query with updated device/EP filters. @@ -217,7 +317,7 @@ void BaseModelCatalog::InvalidateCache() { next_refresh_at_ = std::chrono::steady_clock::time_point{}; } -void BaseModelCatalog::EnsurePopulated(bool allow_refresh) const { +void ModelCatalog::EnsurePopulated(bool allow_refresh) const { // Catalog access is never performance-critical: always take the lock so the // populated_/next_refresh_at_ check and the populate/refresh below are a single // critical section. (No fast path — races on next_refresh_at_ and populated_ are @@ -241,7 +341,7 @@ void BaseModelCatalog::EnsurePopulated(bool allow_refresh) const { next_refresh_at_ = std::chrono::steady_clock::now() + kCacheDuration; } -std::vector BaseModelCatalog::ListModels() const { +std::vector ModelCatalog::ListModels() const { EnsurePopulated(/*allow_refresh=*/true); // PopulateModels appends to models_ under mutex_; iterate under the same lock so a @@ -256,7 +356,7 @@ std::vector BaseModelCatalog::ListModels() const { return result; } -Model* BaseModelCatalog::GetModel(const std::string& alias) const { +Model* ModelCatalog::GetModel(const std::string& alias) const { EnsurePopulated(); auto idx = GetIndex(); @@ -271,7 +371,7 @@ Model* BaseModelCatalog::GetModel(const std::string& alias) const { return nullptr; } -Model* BaseModelCatalog::GetModelVariant(const std::string& model_id) const { +Model* ModelCatalog::GetModelVariant(const std::string& model_id) const { EnsurePopulated(); auto idx = GetIndex(); @@ -322,7 +422,7 @@ Model* BaseModelCatalog::GetModelVariant(const std::string& model_id) const { return nullptr; } -Model* BaseModelCatalog::GetLatestVersion(const Model* model) const { +Model* ModelCatalog::GetLatestVersion(const Model* model) const { if (!model) { return nullptr; } @@ -341,7 +441,7 @@ Model* BaseModelCatalog::GetLatestVersion(const Model* model) const { return nullptr; } -std::vector BaseModelCatalog::GetCachedModels() const { +std::vector ModelCatalog::GetCachedModels() const { EnsurePopulated(); std::lock_guard lock(mutex_); @@ -357,7 +457,7 @@ std::vector BaseModelCatalog::GetCachedModels() const { return result; } -std::vector BaseModelCatalog::GetLoadedModels() const { +std::vector ModelCatalog::GetLoadedModels() const { EnsurePopulated(); std::lock_guard lock(mutex_); @@ -371,9 +471,9 @@ std::vector BaseModelCatalog::GetLoadedModels() const { return result; } -std::vector BaseModelCatalog::GetModelVersions(const std::string& model_alias, - const std::string& variant_name, - int max_versions) { +std::vector ModelCatalog::GetModelVersions(const std::string& model_alias, + const std::string& variant_name, + int max_versions) { if (model_alias.empty()) { FL_THROW(FOUNDRY_LOCAL_ERROR_INVALID_ARGUMENT, "GetModelVersions requires a non-empty model_alias."); } @@ -461,4 +561,40 @@ std::vector BaseModelCatalog::GetModelVersions(const std::string& model_ return result; } +void ModelCatalog::Unregister(const std::string& model_id) { + std::lock_guard lock(mutex_); + + // Locate the container holding this model_id and the preferred variant with that id. + // Variants() is preferred-first, so the first match is the most-preferred (e.g. the local + // BYO copy that shadows a cloud id). + Model* container = nullptr; + Model* target = nullptr; + size_t container_index = 0; + for (size_t i = 0; i < models_.size() && target == nullptr; ++i) { + for (auto* v : models_[i]->Variants()) { + if (v->Info().model_id == model_id) { + container = models_[i].get(); + target = v; + container_index = i; + break; + } + } + } + + if (target == nullptr) { + FL_THROW(FOUNDRY_LOCAL_ERROR_INVALID_ARGUMENT, + fmt::format("Unregister: model_id '{}' not found in the catalog.", model_id)); + } + + container->RemoveVariant(*target); + + // If the container became empty, drop it from stable storage. RemoveVariant clears the + // container's selection when its last variant is removed, so VariantCount() == 0 marks it. + if (container->VariantCount() == 0) { + models_.erase(models_.begin() + static_cast(container_index)); + } + + RebuildIndex(); +} + } // namespace fl diff --git a/sdk_v2/cpp/src/catalog/model_catalog.h b/sdk_v2/cpp/src/catalog/model_catalog.h new file mode 100644 index 000000000..bdd32dd0b --- /dev/null +++ b/sdk_v2/cpp/src/catalog/model_catalog.h @@ -0,0 +1,140 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +#pragma once + +#include "catalog.h" +#include "catalog/model_source.h" +#include "logger.h" + +#include +#include +#include +#include +#include +#include +#include + +namespace fl { + +/// ModelCatalog — the single aggregating store behind ICatalog. +/// +/// Owns the ModelFactory and every Model instance, plus a list of fetch-only IModelSources +/// (one per catalog type). It merges ModelInfo across sources into alias containers, keeps +/// same-model_id copies from different sources as shadow variants (ordered preferred-source +/// first), and serves a filtered preferred-only public API view. Fetch lives in the sources; +/// store / query / index / create / cache / refresh live here. +/// +/// Model ownership: the catalog owns all Model instances via unique_ptr in models_. These +/// pointers are stable for the catalog's lifetime — external code can hold raw Model* safely. +/// The sole exception is Unregister(), an explicit user-initiated removal that frees a variant +/// (see RemoveVariant). Indices (id_index, alias_index, name_index) are rebuilt on refresh but +/// always point into stable storage; id_index resolves each model_id to its preferred leaf via +/// the source-aware variant ordering (first-wins over preferred-first variants_). +/// +/// Was BaseModelCatalog + AzureModelCatalog; the fetch guts moved into AzureModelSource. +class ModelCatalog : public ICatalog { + public: + /// The store owns this factory: sources return ModelInfo (carrying local_path when cached), + /// and the store builds Model leaves from it. + using ModelFactory = std::function; + + ModelCatalog(std::string name, + std::vector> sources, + ModelFactory model_factory, + ILogger& logger); + ~ModelCatalog() override; + + const std::string& GetName() const override { return name_; } + + // ICatalog implementations — query/lookup layer + std::vector ListModels() const override; + Model* GetModel(const std::string& alias) const override; + Model* GetModelVariant(const std::string& model_id) const override; + Model* GetLatestVersion(const Model* model) const override; + std::vector GetCachedModels() const override; + std::vector GetLoadedModels() const override; + std::vector GetModelVersions(const std::string& model_alias, + const std::string& variant_name, + int max_versions = 0) override; + void InvalidateCache() override; + + /// Remove a local-BYO model (foundation for the future BYOM Unregister). Locates the alias + /// container, removes the matching variant, drops the container if it became empty, and + /// rebuilds indices — re-selecting a surviving shadow (e.g. a cloud copy) if any. Not safe to + /// call concurrently with enumeration/model ops on the affected alias. + void Unregister(const std::string& model_id); + + private: + /// Gather ModelInfo from every source (each stamped by Source()) and build Model leaves via + /// the owned factory. Same-model_id shadows from different sources are all built and later + /// grouped into the alias container. + std::vector FetchModels() const; + + /// Gather all versions of an alias from every source and build leaves. + std::vector FetchModelVersions(const std::string& model_alias, + const std::string& model_name = "") const; + + /// Look up specific model IDs across every source and build leaves. + std::vector FetchModelsByIds(const std::vector& model_ids) const; + + /// Build a single Model leaf from a fetched ModelInfo. The info carries local_path when the + /// model is cached locally, which marks the constructed leaf cached. + Model BuildLeaf(ModelInfo info) const; + + /// Lookup indices into the stable models_ storage. + /// Rebuilt on refresh. Does not own any Model instances. + struct ModelIndex { + std::unordered_map id_index; // model_id -> preferred Model* (specific variant) + std::unordered_map alias_index; // alias -> Model* (grouped container) + std::unordered_map name_index; // name -> latest version Model* + }; + + /// Stable model storage. unique_ptr ensures addresses never change. + /// Models are appended on refresh; only Unregister() removes an entry. + mutable std::vector> models_; + + /// Lookup indices, rebuilt on each populate/refresh. + /// Guarded by std::atomic_load/store free functions so readers get a consistent + /// snapshot — the swap after rebuild is atomic, so a concurrent reader never sees + /// a partially-built index. + /// Can't use std::atomic> due to lack of implemention in XCode (macOS) + mutable std::shared_ptr index_; + + /// Atomically grab the current index snapshot. Callers hold the returned shared_ptr + /// for the duration of their lookup, keeping the index alive if a refresh swaps it out. + std::shared_ptr GetIndex() const; + + mutable bool populated_ = false; + mutable std::mutex mutex_; + mutable std::chrono::steady_clock::time_point next_refresh_at_{}; + + static constexpr std::chrono::hours kCacheDuration{4}; + + /// Populate or refresh the catalog (under lock). Groups variants, builds indices. + void PopulateModels(std::vector variants) const; + + /// Merge new variants into the catalog's stable storage. For an existing alias container, + /// appends any variants whose (model_id, catalog_source) isn't already present — allowing + /// same-model_id shadows from different sources. For new aliases, creates a new container. + /// Rebuilds the lookup index when the model set actually changed. + void IntegrateVariants(std::vector variants) const; + + /// Build lookup indices from the current models_ collection. + /// Builds a complete new ModelIndex locally, then atomically swaps it into index_. + void RebuildIndex() const; + + /// Thread-safe access: ensures catalog is populated, refreshes if allowed and stale. + void EnsurePopulated(bool allow_refresh = false) const; + + /// Append-only storage for GetModelVersions query results. Each call appends a new + /// container, so all previously returned Model* pointers remain valid for the catalog's + /// lifetime. These models are intentionally not integrated into the main lookup indices. + mutable std::vector> version_query_models_; + + std::string name_; + std::vector> sources_; + ModelFactory model_factory_; + ILogger& logger_; +}; + +} // namespace fl diff --git a/sdk_v2/cpp/src/catalog/model_source.h b/sdk_v2/cpp/src/catalog/model_source.h new file mode 100644 index 000000000..76e6543e4 --- /dev/null +++ b/sdk_v2/cpp/src/catalog/model_source.h @@ -0,0 +1,50 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +#pragma once + +#include "model_info.h" + +#include +#include + +namespace fl { + +/// IModelSource — a pure, fetch-only contributor to the aggregating ModelCatalog. +/// +/// A source returns `ModelInfo` (never `Model`): it does not own a ModelFactory and never +/// touches DownloadManager / ModelLoadManager. The store creates the `Model` leaves. Each +/// source stamps `info.catalog_source` on every `ModelInfo` it produces so the store can +/// dedup / order shadow variants by source preference, and conveys a cached model's local +/// path via the transient `ModelInfo::local_path` field so the store marks the leaf cached. +/// +/// One source exists per catalog type. Initial scope ships the Public (Azure) source only; +/// Private and BYOM-local sources are future follow-ups that slot in without a store redesign. +class IModelSource { + public: + virtual ~IModelSource() = default; + + /// The catalog source this fetcher serves. Stamped onto every ModelInfo it returns. + virtual CatalogSource Source() const = 0; + + /// Human-readable name (e.g. the catalog URL) for logging / GetName. + virtual std::string Name() const = 0; + + /// Fetch the latest model infos this source offers. Each ModelInfo is stamped with + /// `Source()` (or kLocal for synthesized local stubs) and carries `local_path` when cached. + virtual std::vector FetchModels() const = 0; + + /// Look up specific model IDs (e.g. older versions not in the latest catalog). + /// Default returns `{}` (sources without a by-id lookup). + virtual std::vector FetchModelsByIds(const std::vector& /*model_ids*/) const { + return {}; + } + + /// Fetch all known versions of a model (by alias), bypassing the "latest only" filter. + /// Default returns `{}` (sources that cannot list older versions). + virtual std::vector FetchModelVersions(const std::string& /*model_alias*/, + const std::string& /*model_name*/ = "") const { + return {}; + } +}; + +} // namespace fl diff --git a/sdk_v2/cpp/src/manager.cc b/sdk_v2/cpp/src/manager.cc index 6eb2ea527..2a935bea8 100644 --- a/sdk_v2/cpp/src/manager.cc +++ b/sdk_v2/cpp/src/manager.cc @@ -10,7 +10,8 @@ #include #include "catalog.h" -#include "catalog/azure_model_catalog.h" +#include "catalog/azure_model_source.h" +#include "catalog/model_catalog.h" #include "download/download_manager.h" #if FOUNDRY_LOCAL_HAS_EP_BOOTSTRAPPERS #include "ep_detection/cuda_ep_bootstrapper.h" @@ -322,11 +323,22 @@ Manager::Manager(const Configuration& config) : config_(config) { } catch (...) { logger_->Log(LogLevel::Warning, "telemetry ProcessInfo failed during Manager initialization."); } - catalog_ = std::make_unique( - config_.catalog_urls, download_manager_->GetCacheDirectory(), - [this](ModelInfo info, std::string local_path) { return CreateModel(std::move(info), std::move(local_path)); }, - *ep_detector_, *logger_, config_.external_service_url.has_value(), config_.catalog_region.value_or("auto"), + // Build the source list (initial scope: Public/Azure only) and the aggregating store. + // The Azure source also performs today's inline local-cache resolution. The list is + // source-agnostic, so future Private / BYOM sources slot in without store changes. + auto azure_source = std::make_unique( + config_.catalog_urls, download_manager_->GetCacheDirectory(), *ep_detector_, *logger_, + config_.external_service_url.has_value(), config_.catalog_region.value_or("auto"), disable_region_fallback); + std::string catalog_name = azure_source->Name(); + + std::vector> sources; + sources.push_back(std::move(azure_source)); + + catalog_ = std::make_unique( + std::move(catalog_name), std::move(sources), + [this](ModelInfo info) { return CreateModel(std::move(info)); }, + *logger_); } Manager::~Manager() { @@ -542,8 +554,8 @@ bool Manager::IsShutdownRequested() const { return shutdown_requested_.load(); } const Configuration& Manager::GetConfiguration() const { return config_; } -Model Manager::CreateModel(ModelInfo info, std::string local_path) { - return Model::FromModelInfo(std::move(info), std::move(local_path), *download_manager_, *model_load_manager_); +Model Manager::CreateModel(ModelInfo info) { + return Model::FromModelInfo(std::move(info), *download_manager_, *model_load_manager_); } DownloadManager& Manager::GetDownloadManager() { return *download_manager_; } diff --git a/sdk_v2/cpp/src/manager.h b/sdk_v2/cpp/src/manager.h index a496d865e..d50a730e3 100644 --- a/sdk_v2/cpp/src/manager.h +++ b/sdk_v2/cpp/src/manager.h @@ -150,7 +150,7 @@ class Manager { #endif private: - Model CreateModel(ModelInfo info, std::string local_path); + Model CreateModel(ModelInfo info); static std::mutex s_mutex_; static std::unique_ptr s_instance_; diff --git a/sdk_v2/cpp/src/model.cc b/sdk_v2/cpp/src/model.cc index a4bccb110..006d57291 100644 --- a/sdk_v2/cpp/src/model.cc +++ b/sdk_v2/cpp/src/model.cc @@ -15,6 +15,7 @@ #include #include +#include namespace fl { @@ -70,7 +71,11 @@ int GetModelDevicePriority(const std::string& model_id) { /// 1. Device-type priority (ascending — lower number = better) /// 2. Version number (descending — higher version first) /// 3. CreatedAtUnix timestamp (descending — newer first) -/// 4. model_id (ascending) as final tie-break +/// 4. model_id (ascending) +/// 5. Catalog-source priority (ascending — local > private > public) as the final tiebreak. +/// Genuine same-model_id shadow variants match on all four prior keys, so this key alone +/// decides their relative order (preferred source first). Non-duplicates differ earlier +/// and are unaffected. bool CompareModelsForSort(const Model& m1, const Model& m2) { const auto& info1 = m1.Info(); const auto& info2 = m2.Info(); @@ -93,7 +98,11 @@ bool CompareModelsForSort(const Model& m1, const Model& m2) { return created1 > created2; } - return info1.model_id < info2.model_id; + if (info1.model_id != info2.model_id) { + return info1.model_id < info2.model_id; + } + + return CatalogSourcePriority(info1.catalog_source) < CatalogSourcePriority(info2.catalog_source); } } // namespace @@ -107,7 +116,6 @@ Model::~Model() = default; Model::Model(Model&& other) noexcept : info_(std::move(other.info_)), cached_(other.cached_.load()), - local_path_(std::move(other.local_path_)), download_manager_(other.download_manager_), model_load_manager_(other.model_load_manager_), variants_(std::move(other.variants_)), @@ -122,7 +130,6 @@ Model& Model::operator=(Model&& other) noexcept { if (this != &other) { info_ = std::move(other.info_); cached_.store(other.cached_.load()); - local_path_ = std::move(other.local_path_); download_manager_ = other.download_manager_; model_load_manager_ = other.model_load_manager_; variants_ = std::move(other.variants_); @@ -140,19 +147,19 @@ Model& Model::operator=(Model&& other) noexcept { // --------------------------------------------------------------------------- Model Model::FromModelInfo(ModelInfo info, - std::string local_path, DownloadManager& download_manager, ModelLoadManager& model_load_manager) { Model model; - model.info_ = std::move(info); model.download_manager_ = &download_manager; model.model_load_manager_ = &model_load_manager; - if (!local_path.empty()) { + // ModelInfo owns the cache path; a non-empty local_path means the model is already cached. + if (!info.local_path.empty()) { model.cached_ = true; - model.local_path_ = std::move(local_path); } + model.info_ = std::move(info); + return model; } @@ -250,6 +257,73 @@ std::vector Model::Variants() const { return result; } +std::vector Model::UniqueVariants() const { + std::lock_guard lock(state_mutex_); + + if (!IsContainer()) { + return {const_cast(this)}; + } + + // variants_ is kept best-first (AddVariant's ordered insert), and same-model_id shadow + // variants are ordered preferred-source first by CompareBestFirst's final tiebreak. Keep + // the first occurrence of each model_id so the visible list is the preferred-source copy. + std::vector result; + result.reserve(variants_.size()); + std::unordered_set seen_ids; + for (auto& v : variants_) { + if (seen_ids.insert(v->Info().model_id).second) { + result.push_back(const_cast(v.get())); + } + } + + return result; +} + +size_t Model::VariantCount() const { + std::lock_guard lock(state_mutex_); + return variants_.size(); +} + +void Model::RemoveVariant(const Model& variant) { + if (!IsContainer()) { + FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, "RemoveVariant called on a non-container Model"); + } + + std::lock_guard lock(state_mutex_); + + auto it = std::find_if(variants_.begin(), variants_.end(), + [&variant](const std::unique_ptr& v) { return v.get() == &variant; }); + if (it == variants_.end()) { + FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, "RemoveVariant: variant not found in this model"); + } + + const bool removed_selected = selected_variant_.load(std::memory_order_acquire) == it->get(); + + // Erase-and-compact: every variants_ walker assumes dense, non-null entries, and erase + // preserves the remaining best-first order. + variants_.erase(it); + + if (!removed_selected) { + return; + } + + // The selected leaf was removed — re-run default selection (cached-first, then preferred + // source), or clear the selection if the container is now empty. + if (variants_.empty()) { + selected_variant_.store(nullptr, std::memory_order_release); + return; + } + + for (auto& v : variants_) { + if (v->IsCached()) { + selected_variant_.store(v.get(), std::memory_order_release); + return; + } + } + + selected_variant_.store(variants_.front().get(), std::memory_order_release); +} + bool Model::IsCached() const { if (Model* sv = selected_variant_.load(std::memory_order_acquire)) { return sv->IsCached(); @@ -280,11 +354,11 @@ void Model::Download(std::function progress_cb) { } // Already cached (scanner found the model on disk during catalog construction). - // No need to re-derive the path via DownloadManager — local_path_ is authoritative. + // No need to re-derive the path via DownloadManager — info_.local_path is authoritative. bool already_cached; { std::lock_guard lock(state_mutex_); - already_cached = cached_.load() && !local_path_.empty(); + already_cached = cached_.load() && !info_.local_path.empty(); } if (already_cached) { @@ -299,9 +373,9 @@ void Model::Download(std::function progress_cb) { auto path = download_manager_->DownloadModel(info_, std::move(progress_cb)); { std::lock_guard lock(state_mutex_); - local_path_ = std::move(path); + info_.local_path = std::move(path); } - // local_path_ must be published before cached_ flips true: readers gate on IsCached() + // info_.local_path must be published before cached_ flips true: readers gate on IsCached() // and this release store guarantees they observe the completed path. Do not reorder. cached_.store(true); } @@ -311,7 +385,7 @@ const std::string& Model::GetPath() const { return sv->GetPath(); } - return local_path_; + return info_.local_path; } void Model::Load(ExecutionProvider ep) { @@ -322,10 +396,10 @@ void Model::Load(ExecutionProvider ep) { // LoadModel is idempotent — it returns kModelAlreadyLoaded if the id is already // in the load manager's map, so no need for a local short-circuit. - auto result = model_load_manager_->LoadModel(local_path_, info_.model_id, ep); + auto result = model_load_manager_->LoadModel(info_.local_path, info_.model_id, ep); if (result.status == ModelLoadManager::LoadStatus::kModelNotFound) { - FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, "model not found at path: " + local_path_); + FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, "model not found at path: " + info_.local_path); } } @@ -348,10 +422,10 @@ void Model::RemoveFromCache() { std::string path; { std::lock_guard lock(state_mutex_); - if (!cached_ || local_path_.empty()) { + if (!cached_ || info_.local_path.empty()) { FL_THROW(FOUNDRY_LOCAL_ERROR_INVALID_USAGE, "model is not cached locally"); } - path = local_path_; + path = info_.local_path; } if (IsLoaded()) { @@ -366,7 +440,7 @@ void Model::RemoveFromCache() { cached_.store(false); { std::lock_guard lock(state_mutex_); - local_path_.clear(); + info_.local_path.clear(); } } diff --git a/sdk_v2/cpp/src/model.h b/sdk_v2/cpp/src/model.h index 5c142c830..cda129b54 100644 --- a/sdk_v2/cpp/src/model.h +++ b/sdk_v2/cpp/src/model.h @@ -42,12 +42,12 @@ class Model { Model& operator=(const Model&) = delete; /// Create a leaf Model from a ModelInfo (used when populating the catalog). - /// If local_path is non-empty the model is marked as cached at that location. + /// If `info.local_path` is non-empty the model is marked as cached at that location — + /// ModelInfo is the single owner of the cache path (see model_info.h). /// The managers are non-owning bindings used by Download/Load/Unload. In production /// Manager owns both via unique_ptr. Tests can use `fl::test::FakeServiceBindings` /// (test_helpers.h) for a one-line construction with cheap fakes. static Model FromModelInfo(ModelInfo info, - std::string local_path, DownloadManager& download_manager, ModelLoadManager& model_load_manager); @@ -97,6 +97,13 @@ class Model { /// `std::unique_ptr::get() const → T*` idiom. std::vector Variants() const; + /// Like Variants() but filtered to one leaf per model_id — the visible, de-duplicated view + /// used by the public surfaces (C API GetVariants, REST GET /v1/models). Walks the container's + /// best-first variant order under a single lock and keeps the first occurrence of each model_id, + /// which (per CompareBestFirst's source-priority tiebreak) is the preferred-source copy; later + /// same-model_id shadows are skipped. For a leaf, returns {this}. + std::vector UniqueVariants() const; + // --- Query methods --- bool IsCached() const; @@ -118,6 +125,10 @@ class Model { /// True if this is a multi-variant container. bool IsContainer() const { return selected_variant_.load(std::memory_order_acquire) != nullptr; } + /// Number of variants held by a container. Returns 0 for a leaf (or an emptied container). + /// Used by the store to detect a container that became empty after RemoveVariant. + size_t VariantCount() const; + // --- Mutation methods --- /// Download the model to local cache. @@ -138,6 +149,19 @@ class Model { /// mutated by this call. void SelectVariant(const Model& variant); + /// Remove a variant from this container (used by the local-BYO Unregister path). + /// Finds the matching variant by address, erases and compacts it (no null holes; + /// best-first order preserved), and if the removed leaf was the current selection re-runs + /// default selection (cached-first, then preferred source) over the survivors — or clears + /// the selection if the container is now empty. Throws if this is a leaf or the variant is + /// not part of this container. + /// + /// This is the one place the otherwise append-only variants_ invariant is relaxed. It is a + /// rare, explicit, user-initiated admin op: it frees the erased leaf (invalidating any + /// outstanding Model* to it) and is NOT safe to call concurrently with enumeration or model + /// ops on this container — quiesce first. + void RemoveVariant(const Model& variant); + // --- Non-delegating accessors --- /// Get the local path if cached, or empty string if not. @@ -146,7 +170,7 @@ class Model { /// is invoked on this Model. In practice these mutations are user-initiated /// one-shot operations, so callers reading the path concurrently with download /// or removal of the same Model are out of contract. - const std::string& LocalPath() const { return local_path_; } + const std::string& LocalPath() const { return info_.local_path; } private: // Leaf data (default/empty for containers). @@ -154,13 +178,13 @@ class Model { // Loaded state is NOT stored here; it is queried from ModelLoadManager so the load // manager remains the single source of truth (Manager::Shutdown clears its map without // having to walk every Model and reset a local flag). - // local_path_ is set during Download() (or at construction for already-cached models) and - // cleared by RemoveFromCache(). Its mutation is guarded by state_mutex_; the reader-safety - // contract is that the path is published before cached_ flips true (and cleared after cached_ - // flips false), so any reader that gates on IsCached() observes a complete path. + // The cache path lives in info_.local_path (ModelInfo is its single owner): set during + // Download() (or at construction for already-cached models) and cleared by RemoveFromCache(). + // Its mutation is guarded by state_mutex_; the reader-safety contract is that the path is + // published before cached_ flips true (and cleared after cached_ flips false), so any reader + // that gates on IsCached() observes a complete path. ModelInfo info_; std::atomic cached_{false}; - std::string local_path_; // Non-owning service bindings for leaf operations. Set once at construction and never // reassigned; guaranteed non-null because FromModelInfo takes them by reference. diff --git a/sdk_v2/cpp/src/model_info.cc b/sdk_v2/cpp/src/model_info.cc index cc2f68f16..f9edc2eac 100644 --- a/sdk_v2/cpp/src/model_info.cc +++ b/sdk_v2/cpp/src/model_info.cc @@ -5,6 +5,7 @@ #include #include +#include #include namespace fl { @@ -106,6 +107,21 @@ ModelInfo ModelInfoFromJson(const nlohmann::json& j) { info.detected_region = j["detectedRegion"].get(); } + // catalogSource — round-trips the serving catalog source. Absent → kPublic (default). + if (j.contains("catalogSource") && j["catalogSource"].is_number_integer()) { + switch (j["catalogSource"].get()) { + case static_cast(CatalogSource::kPrivate): + info.catalog_source = CatalogSource::kPrivate; + break; + case static_cast(CatalogSource::kLocal): + info.catalog_source = CatalogSource::kLocal; + break; + default: + info.catalog_source = CatalogSource::kPublic; + break; + } + } + // String properties — named top-level fields → string_properties map ReadStringProp(j, "providerType", info.string_properties, FOUNDRY_LOCAL_MODEL_PROP_MODEL_PROVIDER_STR); ReadStringProp(j, "modelType", info.string_properties, FOUNDRY_LOCAL_MODEL_PROP_MODEL_TYPE_STR); @@ -185,6 +201,14 @@ ModelInfo ModelInfoFromJson(const nlohmann::json& j) { // "cached" is ignored on deserialize — Model tracks this separately. + // localPath — runtime cache location. Only present in the on-disk snapshot when it existed at + // save time (see ModelInfoToJson). A fresh disk scan (AddLocalModels) overrides this on every + // fetch, so a since-deleted path is corrected for models still present; treat as a best-effort + // hint only. Local cache support is intentionally lightweight (BYOM will supersede it). + if (j.contains("localPath") && j["localPath"].is_string()) { + info.local_path = j["localPath"].get(); + } + return info; } @@ -208,6 +232,12 @@ nlohmann::json ModelInfoToJson(const ModelInfo& info) { j["detectedRegion"] = info.detected_region; } + // catalogSource — emit as an int so the serving source survives the on-disk cache. + // Only emitted for non-public sources to keep public-catalog snapshots byte-stable. + if (info.catalog_source != CatalogSource::kPublic) { + j["catalogSource"] = static_cast(info.catalog_source); + } + // providerType — required in C#, defaults to empty const auto* provider = info.GetPropertyStr(FOUNDRY_LOCAL_MODEL_PROP_MODEL_PROVIDER_STR); j["providerType"] = provider ? *provider : ""; @@ -339,6 +369,14 @@ nlohmann::json ModelInfoToJson(const ModelInfo& info) { j["testModel"] = (*test_model != 0); } + // localPath — runtime cache location. Validate it still exists on disk before persisting so a + // stale snapshot never advertises a since-deleted model as cached. This is best-effort: a path + // valid now can be deleted before the snapshot is next loaded, but AddLocalModels re-scans disk + // on every fetch and overrides this for models still present. + if (!info.local_path.empty() && std::filesystem::exists(info.local_path)) { + j["localPath"] = info.local_path; + } + return j; } diff --git a/sdk_v2/cpp/src/model_info.h b/sdk_v2/cpp/src/model_info.h index 09f279545..1dab5363c 100644 --- a/sdk_v2/cpp/src/model_info.h +++ b/sdk_v2/cpp/src/model_info.h @@ -29,6 +29,35 @@ enum class DeviceType { /// Returns "CPU"/"GPU"/"NPU" or "Invalid" for kNotSet. std::string DeviceTypeToString(DeviceType dt); +/// Which catalog served a model. Drives shadow-variant dedup and source preference +/// when the same model_id is offered by more than one catalog source. +/// +/// `kPublic = 0` so that a zero-initialized / legacy `ModelInfo` (and any cache JSON +/// without a `catalogSource` field) decodes as the public Azure catalog. The numeric +/// order of the enum is NOT the preference order — preference is expressed by +/// `CatalogSourcePriority` (local ranks most-preferred). +enum class CatalogSource { + kPublic = 0, ///< Public Azure catalog. + kPrivate = 1, ///< Private online catalog (future follow-up). + kLocal = 2, ///< BYOM / disk-only stub (short-term marker until the BYOM catalog lands). +}; + +/// Preference rank for a catalog source: lower = more preferred (local > private > public). +/// Used as the final tiebreak in `Model::CompareBestFirst` so it only ever reorders genuine +/// same-`model_id` duplicates. Independent of the enum's underlying value order. +inline int CatalogSourcePriority(CatalogSource source) { + switch (source) { + case CatalogSource::kLocal: + return 0; + case CatalogSource::kPrivate: + return 1; + case CatalogSource::kPublic: + return 2; + } + + return 3; +} + struct ModelInfo { std::string model_id; std::string name; @@ -40,6 +69,19 @@ struct ModelInfo { std::string execution_provider; // e.g. "WebGPUExecutionProvider", empty if not set std::string task; + // Which catalog source served this model. Drives shadow-variant dedup and preference. + // Defaults to kPublic so zero-initialized / legacy infos decode as public. + // Round-trips through the on-disk catalog cache (absent → kPublic). + CatalogSource catalog_source = CatalogSource::kPublic; + + // Local cache path for this model. ModelInfo is the single owner of the path: it is populated + // by a source when the model is found in the local cache, set by Model::Download() after a + // successful download, and cleared by Model::RemoveFromCache(). Empty for uncached models. + // Runtime-only and not exposed via the C API. It is persisted to the on-disk catalog snapshot + // only when it still exists on disk at save time (ModelInfoToJson validates), and a fresh disk + // scan overrides it on every fetch — so treat any deserialized value as a best-effort hint. + std::string local_path; + // Azure region the catalog was served from (auto-detected from cluster headers). // Empty for non-Azure / BYO models. Used to target the matching regional model // registry when downloading. Round-trips through the on-disk catalog cache. diff --git a/sdk_v2/cpp/src/service/models_handlers.cc b/sdk_v2/cpp/src/service/models_handlers.cc index 5cd6ee8ea..a464dccf5 100644 --- a/sdk_v2/cpp/src/service/models_handlers.cc +++ b/sdk_v2/cpp/src/service/models_handlers.cc @@ -161,8 +161,9 @@ class OpenAIListModelsHandler : public HttpRequestHandler { nlohmann::json data = nlohmann::json::array(); // List individual variants so the client knows exactly which model_id to use. + // UniqueVariants() de-duplicates shadow model_ids to the preferred-source copy. for (const auto* model : models) { - for (const auto* variant : model->Variants()) { + for (const auto* variant : model->UniqueVariants()) { const auto& info = variant->Info(); int64_t created = 0; auto it = info.int_properties.find(FOUNDRY_LOCAL_MODEL_PROP_CREATED_AT_UNIX_INT); diff --git a/sdk_v2/cpp/test/CMakeLists.txt b/sdk_v2/cpp/test/CMakeLists.txt index 5b53c4bbc..d3e31c1f9 100644 --- a/sdk_v2/cpp/test/CMakeLists.txt +++ b/sdk_v2/cpp/test/CMakeLists.txt @@ -11,7 +11,7 @@ add_executable(foundry_local_tests internal_api/audio/audio_session_test.cc internal_api/audio/audio_transcription_contract_test.cc internal_api/audio/pcm_utils_test.cc - internal_api/base_model_catalog_test.cc + internal_api/model_catalog_test.cc internal_api/blob_download_state_test.cc internal_api/c_api_test.cc internal_api/callback_handler_test.cc @@ -52,7 +52,7 @@ add_executable(foundry_local_tests internal_api/region_fallback_test.cc internal_api/sse_stream_body_test.cc internal_api/azure_catalog_test.cc - internal_api/azure_model_catalog_test.cc + internal_api/azure_model_source_test.cc internal_api/telemetry_test.cc internal_api/tensor_test.cc internal_api/chat/search_options_test.cc diff --git a/sdk_v2/cpp/test/internal_api/audio/audio_session_test.cc b/sdk_v2/cpp/test/internal_api/audio/audio_session_test.cc index d0dff10d9..2ba1fef3c 100644 --- a/sdk_v2/cpp/test/internal_api/audio/audio_session_test.cc +++ b/sdk_v2/cpp/test/internal_api/audio/audio_session_test.cc @@ -257,7 +257,7 @@ class AudioSessionTest : public ::testing::Test { static inline GenAIModelInstance* model_ = nullptr; static inline fl::test::FakeServiceBindings svc_; static inline Model catalog_model_ = Model::FromModelInfo( - ModelInfo{}, "", svc_.download_manager, svc_.model_load_manager); + ModelInfo{}, svc_.download_manager, svc_.model_load_manager); TelemetryLogger null_telemetry_{"test", fl::test::NullLog()}; fl::test::NullSessionManager null_session_manager_; }; @@ -309,7 +309,7 @@ class AudioSessionInferenceTest : public ::testing::Test { static inline GenAIModelInstance* model_ = nullptr; static inline fl::test::FakeServiceBindings svc_; static inline Model catalog_model_ = Model::FromModelInfo( - ModelInfo{}, "", svc_.download_manager, svc_.model_load_manager); + ModelInfo{}, svc_.download_manager, svc_.model_load_manager); TelemetryLogger null_telemetry_{"test", fl::test::NullLog()}; fl::test::NullSessionManager null_session_manager_; }; @@ -377,7 +377,7 @@ class AudioSessionNemotronInferenceTest : public ::testing::Test { static inline std::string nemotron_alias_; static inline fl::test::FakeServiceBindings svc_; static inline Model catalog_model_ = Model::FromModelInfo( - ModelInfo{}, "", svc_.download_manager, svc_.model_load_manager); + ModelInfo{}, svc_.download_manager, svc_.model_load_manager); TelemetryLogger null_telemetry_{"test", fl::test::NullLog()}; fl::test::NullSessionManager null_session_manager_; }; diff --git a/sdk_v2/cpp/test/internal_api/azure_model_catalog_test.cc b/sdk_v2/cpp/test/internal_api/azure_model_source_test.cc similarity index 64% rename from sdk_v2/cpp/test/internal_api/azure_model_catalog_test.cc rename to sdk_v2/cpp/test/internal_api/azure_model_source_test.cc index 77179bd00..d9c7eb2a5 100644 --- a/sdk_v2/cpp/test/internal_api/azure_model_catalog_test.cc +++ b/sdk_v2/cpp/test/internal_api/azure_model_source_test.cc @@ -1,11 +1,15 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +// +// Tests for AzureModelSource — the Public (Azure) catalog source (fetch guts ported from the +// former AzureModelCatalog). A source returns ModelInfo only: it stamps catalog_source, attaches +// each cached entry's transient local_path, synthesizes kLocal stubs for disk-only models, falls +// back to the on-disk snapshot when every live URL fails, and saves the live snapshot. -#include "catalog/azure_model_catalog.h" +#include "catalog/azure_model_source.h" #include "catalog/catalog_cache.h" #include "catalog/catalog_client.h" #include "internal_api/test_helpers.h" -#include "model.h" #include "model_info.h" #include "utils/temp_path.h" @@ -16,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -72,20 +77,18 @@ class FakeCatalogClient final : public ICatalogClient { std::shared_ptr behavior_; }; -class TestAzureModelCatalog final : public AzureModelCatalog { +class TestAzureModelSource final : public AzureModelSource { public: using ClientFactory = std::function(const std::string& url, const std::string& filter)>; - TestAzureModelCatalog(std::vector>> catalog_urls, - std::string cache_dir, - ModelFactory model_factory, - const IEpDetector& ep_detector, - ILogger& logger, - bool cache_only, - ClientFactory client_factory) - : AzureModelCatalog(std::move(catalog_urls), std::move(cache_dir), std::move(model_factory), ep_detector, logger, - cache_only), + TestAzureModelSource(std::vector>> catalog_urls, + std::string cache_dir, + const IEpDetector& ep_detector, + ILogger& logger, + bool cache_only, + ClientFactory client_factory) + : AzureModelSource(std::move(catalog_urls), std::move(cache_dir), ep_detector, logger, cache_only), client_factory_(std::move(client_factory)) {} protected: @@ -114,12 +117,10 @@ ModelInfo MakeModelInfo(const std::string& model_id, return info; } -Model* FindVariant(const std::vector& models, const std::string& model_id) { - for (auto* model : models) { - for (auto* variant : model->Variants()) { - if (variant->Info().model_id == model_id) { - return variant; - } +const ModelInfo* FindInfo(const std::vector& infos, const std::string& model_id) { + for (const auto& info : infos) { + if (info.model_id == model_id) { + return &info; } } @@ -128,7 +129,7 @@ Model* FindVariant(const std::vector& models, const std::string& model_i } // namespace -class AzureModelCatalogTest : public ::testing::Test { +class AzureModelSourceTest : public ::testing::Test { protected: std::shared_ptr AddBehavior(const std::string& url, bool fail_fetch_all = false) { auto behavior = std::make_shared(); @@ -171,15 +172,11 @@ class AzureModelCatalogTest : public ::testing::Test { file << snapshot.dump(2); } - std::unique_ptr CreateCatalog( + std::unique_ptr CreateSource( std::vector>> catalog_urls, bool cache_only = false) { - auto model_factory = [this](ModelInfo info, std::string local_path) { - return Model::FromModelInfo(std::move(info), std::move(local_path), services_.download_manager, - services_.model_load_manager); - }; auto client_factory = [this](const std::string& url, const std::string&) { - ++factory_calls_; + ++client_creations_; auto behavior = behaviors_.find(url); if (behavior == behaviors_.end()) { throw std::runtime_error("missing fake behavior for " + url); @@ -188,22 +185,20 @@ class AzureModelCatalogTest : public ::testing::Test { return std::make_unique(behavior->second); }; - return std::make_unique(std::move(catalog_urls), cache_directory_.string(), - std::move(model_factory), services_.ep_detector, services_.logger, - cache_only, std::move(client_factory)); + return std::make_unique(std::move(catalog_urls), cache_directory_.string(), + services_.ep_detector, services_.logger, cache_only, + std::move(client_factory)); } - fl::test::TempPath cache_directory_ = fl::test::TempPath::CreateTempDir("fl_azure_model_catalog_"); + fl::test::TempPath cache_directory_ = fl::test::TempPath::CreateTempDir("fl_azure_model_source_"); fl::test::FakeServiceBindings services_; std::unordered_map> behaviors_; - int factory_calls_ = 0; + int client_creations_ = 0; }; -TEST_F(AzureModelCatalogTest, AllUrlsFailUsesSnapshotMetadataAndScannedPathsWithoutRewritingSnapshot) { - const auto gpu_info = - MakeModelInfo("snapshot-gpu:2", "snapshot-gpu", 2, "snapshot-alias", "SnapshotProvider"); - const auto cpu_info = - MakeModelInfo("snapshot-cpu:2", "snapshot-cpu", 2, "snapshot-alias", "SnapshotProvider"); +TEST_F(AzureModelSourceTest, AllUrlsFailUsesSnapshotMetadataAndScannedPathsWithoutRewritingSnapshot) { + const auto gpu_info = MakeModelInfo("snapshot-gpu:2", "snapshot-gpu", 2, "snapshot-alias", "SnapshotProvider"); + const auto cpu_info = MakeModelInfo("snapshot-cpu:2", "snapshot-cpu", 2, "snapshot-alias", "SnapshotProvider"); WriteSnapshot({gpu_info, cpu_info}); const auto gpu_path = AddLocalModel("snapshot-gpu:2", "snapshot-gpu"); const auto cpu_path = AddLocalModel("snapshot-cpu:2", "snapshot-cpu"); @@ -211,30 +206,31 @@ TEST_F(AzureModelCatalogTest, AllUrlsFailUsesSnapshotMetadataAndScannedPathsWith AddBehavior("https://catalog-one.test", true); AddBehavior("https://catalog-two.test", true); - auto catalog = CreateCatalog({ + auto source = CreateSource({ {"https://catalog-one.test", std::nullopt}, {"https://catalog-two.test", std::nullopt}, }); - const auto cached_models = catalog->GetCachedModels(); + const auto infos = source->FetchModels(); - ASSERT_EQ(cached_models.size(), 3u); - auto* gpu_model = FindVariant(cached_models, "snapshot-gpu:2"); - ASSERT_NE(gpu_model, nullptr); - EXPECT_EQ(gpu_model->Info().alias, "snapshot-alias"); - const auto* provider = gpu_model->Info().GetPropertyStr(FOUNDRY_LOCAL_MODEL_PROP_MODEL_PROVIDER_STR); + ASSERT_EQ(infos.size(), 3u); + const auto* gpu = FindInfo(infos, "snapshot-gpu:2"); + ASSERT_NE(gpu, nullptr); + EXPECT_EQ(gpu->alias, "snapshot-alias"); + EXPECT_EQ(gpu->catalog_source, CatalogSource::kPublic); + const auto* provider = gpu->GetPropertyStr(FOUNDRY_LOCAL_MODEL_PROP_MODEL_PROVIDER_STR); ASSERT_NE(provider, nullptr); EXPECT_EQ(*provider, "SnapshotProvider"); - EXPECT_EQ(gpu_model->LocalPath(), gpu_path.string()); + EXPECT_EQ(gpu->local_path, gpu_path.string()); - auto* cpu_model = FindVariant(cached_models, "snapshot-cpu:2"); - ASSERT_NE(cpu_model, nullptr); - EXPECT_EQ(cpu_model->Info().alias, "snapshot-alias"); - EXPECT_EQ(cpu_model->LocalPath(), cpu_path.string()); + const auto* cpu = FindInfo(infos, "snapshot-cpu:2"); + ASSERT_NE(cpu, nullptr); + EXPECT_EQ(cpu->local_path, cpu_path.string()); - auto* byom_model = FindVariant(cached_models, "offline-byom:4"); - ASSERT_NE(byom_model, nullptr); - EXPECT_EQ(byom_model->LocalPath(), byom_path.string()); + const auto* byom = FindInfo(infos, "offline-byom:4"); + ASSERT_NE(byom, nullptr); + EXPECT_EQ(byom->catalog_source, CatalogSource::kLocal); + EXPECT_EQ(byom->local_path, byom_path.string()); CatalogCache persisted_cache(cache_directory_.string(), services_.logger); persisted_cache.Load(); @@ -243,68 +239,69 @@ TEST_F(AzureModelCatalogTest, AllUrlsFailUsesSnapshotMetadataAndScannedPathsWith ASSERT_EQ(persisted_models->size(), 2u); EXPECT_EQ((*persisted_models)[0].model_id, "snapshot-gpu:2"); EXPECT_EQ((*persisted_models)[1].model_id, "snapshot-cpu:2"); - EXPECT_EQ(factory_calls_, 2); + EXPECT_EQ(client_creations_, 2); } -TEST_F(AzureModelCatalogTest, AllUrlsFailWithoutSnapshotSurfacesScannedModelAsByom) { +TEST_F(AzureModelSourceTest, AllUrlsFailWithoutSnapshotSurfacesScannedModelAsByom) { const auto local_path = AddLocalModel("custom-model:0", "custom-model"); AddBehavior("https://catalog-one.test", true); AddBehavior("https://catalog-two.test", true); - auto catalog = CreateCatalog({ + auto source = CreateSource({ {"https://catalog-one.test", std::nullopt}, {"https://catalog-two.test", std::nullopt}, }); - const auto cached_models = catalog->GetCachedModels(); - - ASSERT_EQ(cached_models.size(), 1u); - auto* byom_model = FindVariant(cached_models, "custom-model:0"); - ASSERT_NE(byom_model, nullptr); - EXPECT_EQ(byom_model->Info().name, "custom-model"); - EXPECT_EQ(byom_model->Info().alias, "custom-model"); - EXPECT_EQ(byom_model->Info().version, 0); - EXPECT_EQ(byom_model->Info().uri, "local://custom-model"); - const auto* provider = byom_model->Info().GetPropertyStr(FOUNDRY_LOCAL_MODEL_PROP_MODEL_PROVIDER_STR); - const auto* model_type = byom_model->Info().GetPropertyStr(FOUNDRY_LOCAL_MODEL_PROP_MODEL_TYPE_STR); + const auto infos = source->FetchModels(); + + ASSERT_EQ(infos.size(), 1u); + const auto* byom = FindInfo(infos, "custom-model:0"); + ASSERT_NE(byom, nullptr); + EXPECT_EQ(byom->name, "custom-model"); + EXPECT_EQ(byom->alias, "custom-model"); + EXPECT_EQ(byom->version, 0); + EXPECT_EQ(byom->uri, "local://custom-model"); + EXPECT_EQ(byom->catalog_source, CatalogSource::kLocal); + const auto* provider = byom->GetPropertyStr(FOUNDRY_LOCAL_MODEL_PROP_MODEL_PROVIDER_STR); + const auto* model_type = byom->GetPropertyStr(FOUNDRY_LOCAL_MODEL_PROP_MODEL_TYPE_STR); ASSERT_NE(provider, nullptr); ASSERT_NE(model_type, nullptr); EXPECT_EQ(*provider, "Local"); EXPECT_EQ(*model_type, "ONNX"); - EXPECT_EQ(byom_model->LocalPath(), local_path.string()); + EXPECT_EQ(byom->local_path, local_path.string()); EXPECT_FALSE(fs::exists(cache_directory_.path() / "foundry.modelinfo.json")); } -TEST_F(AzureModelCatalogTest, EmptySuccessfulUrlPreventsSnapshotFallbackWhenAnotherUrlFails) { +TEST_F(AzureModelSourceTest, EmptySuccessfulUrlPreventsSnapshotFallbackWhenAnotherUrlFails) { WriteSnapshot({MakeModelInfo("snapshot-only:1", "snapshot-only", 1, "snapshot-only", "SnapshotProvider")}); AddBehavior("https://failed-catalog.test", true); const auto successful_behavior = AddBehavior("https://empty-catalog.test"); - auto catalog = CreateCatalog({ + auto source = CreateSource({ {"https://failed-catalog.test", std::nullopt}, {"https://empty-catalog.test", std::nullopt}, }); - const auto models = catalog->ListModels(); + const auto infos = source->FetchModels(); - EXPECT_TRUE(models.empty()); + EXPECT_TRUE(infos.empty()); EXPECT_EQ(successful_behavior->fetch_all_calls, 1); - EXPECT_EQ(factory_calls_, 2); + EXPECT_EQ(client_creations_, 2); } -TEST_F(AzureModelCatalogTest, CacheOnlyUsesSnapshotAndScannedByomWithoutCreatingLiveClient) { +TEST_F(AzureModelSourceTest, CacheOnlyUsesSnapshotAndScannedByomWithoutCreatingLiveClient) { WriteSnapshot({MakeModelInfo("snapshot-model:3", "snapshot-model", 3, "snapshot-alias", "SnapshotProvider")}); AddLocalModel("snapshot-model:3", "snapshot-model"); AddLocalModel("cache-only-byom:5", "cache-only-byom"); - auto catalog = CreateCatalog({{"https://must-not-be-called.test", std::nullopt}}, true); + auto source = CreateSource({{"https://must-not-be-called.test", std::nullopt}}, true); - const auto cached_models = catalog->GetCachedModels(); + const auto infos = source->FetchModels(); - ASSERT_EQ(cached_models.size(), 2u); - EXPECT_NE(FindVariant(cached_models, "snapshot-model:3"), nullptr); - EXPECT_NE(FindVariant(cached_models, "cache-only-byom:5"), nullptr); - EXPECT_EQ(factory_calls_, 0); + ASSERT_EQ(infos.size(), 2u); + EXPECT_NE(FindInfo(infos, "snapshot-model:3"), nullptr); + EXPECT_NE(FindInfo(infos, "cache-only-byom:5"), nullptr); + EXPECT_EQ(client_creations_, 0); } -TEST_F(AzureModelCatalogTest, LiveAggregationDeduplicatesAndSavesResolvedAndByomMetadata) { +TEST_F(AzureModelSourceTest, LiveAggregationDeduplicatesAndSavesResolvedAndByomMetadata) { AddLocalModel("old-model:1", "old-model"); AddLocalModel("custom-model:0", "custom-model"); @@ -317,16 +314,22 @@ TEST_F(AzureModelCatalogTest, LiveAggregationDeduplicatesAndSavesResolvedAndByom const auto second_behavior = AddBehavior("https://catalog-two.test"); second_behavior->all_models = {latest_info}; - auto catalog = CreateCatalog({ + auto source = CreateSource({ {"https://catalog-one.test", std::nullopt}, {"https://catalog-two.test", std::nullopt}, }); - const auto models = catalog->ListModels(); + const auto infos = source->FetchModels(); - ASSERT_EQ(models.size(), 3u); + ASSERT_EQ(infos.size(), 3u); EXPECT_EQ(first_behavior->fetch_by_id_calls, 1); EXPECT_EQ(second_behavior->fetch_by_id_calls, 1); + EXPECT_NE(FindInfo(infos, "latest-model:2"), nullptr); + EXPECT_NE(FindInfo(infos, "old-model:1"), nullptr); + + const auto* byom = FindInfo(infos, "custom-model:0"); + ASSERT_NE(byom, nullptr); + EXPECT_EQ(byom->catalog_source, CatalogSource::kLocal); CatalogCache persisted_cache(cache_directory_.string(), services_.logger); persisted_cache.Load(); diff --git a/sdk_v2/cpp/test/internal_api/base_model_catalog_test.cc b/sdk_v2/cpp/test/internal_api/base_model_catalog_test.cc deleted file mode 100644 index 5455031d1..000000000 --- a/sdk_v2/cpp/test/internal_api/base_model_catalog_test.cc +++ /dev/null @@ -1,448 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. -// -// Tests for BaseModelCatalog's lookup methods after variant grouping: -// - ListModels: returns Model containers (one per alias group) -// - GetModel: by name/alias (returns Model container) -// - GetModelVariant: by model_id (returns specific variant) -// - GetCachedModels / GetLoadedModels -// - Variant grouping behavior -// -#include "catalog/base_model_catalog.h" -#include "internal_api/test_helpers.h" -#include "logger.h" -#include "model.h" -#include "model_info.h" - -#include - -#include -#include -#include - -using namespace fl; - -static Model MakeModel(const std::string& model_id, const std::string& name, - int version, const std::string& alias, - const std::string& local_path); - -// ======================================================================== -// Concrete test catalog — returns canned models from FetchModels() -// ======================================================================== - -class TestCatalog : public BaseModelCatalog { - public: - explicit TestCatalog(ILogger& logger) : BaseModelCatalog("test-catalog", logger) {} - - void AddModel(Model model) { - models_.push_back(std::move(model)); - } - - protected: - std::vector FetchModels() const override { - return std::move(models_); - } - - private: - mutable std::vector models_; -}; - -class QueryingTestCatalog : public BaseModelCatalog { - public: - explicit QueryingTestCatalog(ILogger& logger) : BaseModelCatalog("querying-test-catalog", logger) {} - - void AddModel(Model model) { - models_.push_back(std::move(model)); - } - - void SetVersionFetchResults(std::vector models) { - version_fetch_results_ = std::move(models); - } - - void SetIdFetchResults(std::vector models) { - id_fetch_results_ = std::move(models); - } - - protected: - std::vector FetchModels() const override { - return std::move(models_); - } - - std::vector FetchModelVersions(const std::string& model_alias, - const std::string& model_name = "") const override { - std::vector result; - for (const auto& model : version_fetch_results_) { - const auto& info = model.Info(); - if (info.alias != model_alias) { - continue; - } - - if (!model_name.empty() && info.name != model_name) { - continue; - } - - result.push_back(MakeModel(info.model_id, info.name, info.version, info.alias, model.LocalPath())); - } - - return result; - } - - std::vector FetchModelsByIds(const std::vector& model_ids) const override { - std::unordered_set requested(model_ids.begin(), model_ids.end()); - std::vector result; - for (const auto& model : id_fetch_results_) { - const auto& info = model.Info(); - if (!requested.contains(info.model_id)) { - continue; - } - - result.push_back(MakeModel(info.model_id, info.name, info.version, info.alias, model.LocalPath())); - } - - return result; - } - - private: - mutable std::vector models_; - mutable std::vector version_fetch_results_; - mutable std::vector id_fetch_results_; -}; - -// Helper: create a Model from basic fields. -static Model MakeModel(const std::string& model_id, const std::string& name, - int version, const std::string& alias, - const std::string& local_path = {}) { - static fl::test::FakeServiceBindings svc; - ModelInfo info; - info.model_id = model_id; - info.name = name; - info.version = version; - info.alias = alias; - return Model::FromModelInfo(std::move(info), local_path, - svc.download_manager, svc.model_load_manager); -} - -// ======================================================================== -// Test fixture -// ======================================================================== - -class BaseModelCatalogTest : public ::testing::Test { - protected: - StderrLogger logger_; -}; - -// ======================================================================== -// GetName -// ======================================================================== - -TEST_F(BaseModelCatalogTest, GetName_ReturnsNameFromConstruction) { - TestCatalog catalog(logger_); - EXPECT_EQ(catalog.GetName(), "test-catalog"); -} - -// ======================================================================== -// GetModel -// ======================================================================== - -TEST_F(BaseModelCatalogTest, GetModel_ByName_ReturnsNullptr) { - TestCatalog catalog(logger_); - catalog.AddModel(MakeModel("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); - - // GetModel only matches by alias, not by name. - EXPECT_EQ(catalog.GetModel("phi-3-mini"), nullptr); -} - -TEST_F(BaseModelCatalogTest, GetModel_ByAlias) { - TestCatalog catalog(logger_); - catalog.AddModel(MakeModel("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); - - Model* m = catalog.GetModel("phi-3"); - ASSERT_NE(m, nullptr); - EXPECT_EQ(m->Info().model_id, "phi-3-mini:1"); -} - -TEST_F(BaseModelCatalogTest, GetModel_Nonexistent_ReturnsNullptr) { - TestCatalog catalog(logger_); - catalog.AddModel(MakeModel("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); - - EXPECT_EQ(catalog.GetModel("nonexistent"), nullptr); -} - -TEST_F(BaseModelCatalogTest, GetModel_EmptyCatalog_ReturnsNullptr) { - TestCatalog catalog(logger_); - - EXPECT_EQ(catalog.GetModel("anything"), nullptr); -} - -// ======================================================================== -// GetModelVariant -// ======================================================================== - -TEST_F(BaseModelCatalogTest, GetModelVariant_ById) { - TestCatalog catalog(logger_); - catalog.AddModel(MakeModel("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); - - Model* m = catalog.GetModelVariant("phi-3-mini:1"); - ASSERT_NE(m, nullptr); - EXPECT_EQ(m->Info().model_id, "phi-3-mini:1"); -} - -TEST_F(BaseModelCatalogTest, GetModelVariant_Nonexistent_ReturnsNullptr) { - TestCatalog catalog(logger_); - catalog.AddModel(MakeModel("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); - - EXPECT_EQ(catalog.GetModelVariant("nonexistent"), nullptr); -} - -// ======================================================================== -// GetModel — variants accessible via container -// ======================================================================== - -TEST_F(BaseModelCatalogTest, GetModel_VariantsAccessible) { - TestCatalog catalog(logger_); - catalog.AddModel(MakeModel("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); - catalog.AddModel(MakeModel("phi-3-mini:2", "phi-3-mini", 2, "phi-3")); - - Model* container = catalog.GetModel("phi-3"); - ASSERT_NE(container, nullptr); - EXPECT_EQ(container->Variants().size(), 2u); -} - -TEST_F(BaseModelCatalogTest, GetModel_NotFound_ReturnsNullptr) { - TestCatalog catalog(logger_); - catalog.AddModel(MakeModel("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); - - EXPECT_EQ(catalog.GetModel("nonexistent-alias"), nullptr); -} - -TEST_F(BaseModelCatalogTest, GetModel_EmptyString_ReturnsNullptr) { - TestCatalog catalog(logger_); - catalog.AddModel(MakeModel("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); - - EXPECT_EQ(catalog.GetModel(""), nullptr); -} - -// ======================================================================== -// ListModels — returns grouped Model containers -// ======================================================================== - -TEST_F(BaseModelCatalogTest, ListModels_ReturnsGroupedByAlias) { - TestCatalog catalog(logger_); - catalog.AddModel(MakeModel("a:1", "a", 1, "a")); - catalog.AddModel(MakeModel("b:1", "b", 1, "b")); - - auto list = catalog.ListModels(); - EXPECT_EQ(list.size(), 2u); -} - -TEST_F(BaseModelCatalogTest, ListModels_VariantsGroupedIntoSingleModel) { - TestCatalog catalog(logger_); - catalog.AddModel(MakeModel("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); - catalog.AddModel(MakeModel("phi-3-mini:2", "phi-3-mini", 2, "phi-3")); - - // Two variants with same alias → one Model container - auto list = catalog.ListModels(); - EXPECT_EQ(list.size(), 1u); - EXPECT_EQ(list[0]->Alias(), "phi-3"); - - // The Model container has 2 variants - const auto& variants = list[0]->Variants(); - EXPECT_EQ(variants.size(), 2u); -} - -TEST_F(BaseModelCatalogTest, ListModels_MultipleAliasGroups) { - TestCatalog catalog(logger_); - catalog.AddModel(MakeModel("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); - catalog.AddModel(MakeModel("phi-3-mini:2", "phi-3-mini", 2, "phi-3")); - catalog.AddModel(MakeModel("llama:1", "llama", 1, "llama")); - - // 2 alias groups: phi-3 (2 variants) and llama (1 variant) - auto list = catalog.ListModels(); - EXPECT_EQ(list.size(), 2u); -} - -// ======================================================================== -// BuildFromVariants — models with missing fields are skipped -// ======================================================================== - -TEST_F(BaseModelCatalogTest, GetModel_SkipsInvalidEntries) { - // Model with empty model_id should be skipped during grouping - ModelInfo invalid_info; - invalid_info.model_id = ""; // missing - invalid_info.name = "bad"; - invalid_info.alias = "bad-alias"; - - TestCatalog catalog(logger_); - fl::test::FakeServiceBindings svc; - catalog.AddModel(Model::FromModelInfo(invalid_info, "", - svc.download_manager, svc.model_load_manager)); - catalog.AddModel(MakeModel("good:1", "good", 1, "good-alias")); - - // Invalid model is skipped during grouping — only the valid model is listed - auto list = catalog.ListModels(); - EXPECT_EQ(list.size(), 1u); - - // Only the valid model is findable by id/alias - EXPECT_NE(catalog.GetModelVariant("good:1"), nullptr); - EXPECT_EQ(catalog.GetModel("bad"), nullptr); -} - -// ======================================================================== -// Variant grouping — cached variant preference -// ======================================================================== - -TEST_F(BaseModelCatalogTest, GroupedModel_PrefersCachedVariant) { - TestCatalog catalog(logger_); - catalog.AddModel(MakeModel("phi-3:1", "phi-3", 1, "phi-3")); // not cached - catalog.AddModel(MakeModel("phi-3:2", "phi-3", 2, "phi-3", "/path/to/cached/model")); // cached - - auto* m = catalog.GetModel("phi-3"); - ASSERT_NE(m, nullptr); - - // The selected variant should be the cached one (phi-3:2) - EXPECT_TRUE(m->IsCached()); - EXPECT_EQ(m->Info().model_id, "phi-3:2"); -} - -TEST_F(BaseModelCatalogTest, GetCachedModelsReturnsEveryCachedLeafInCatalogVariantOrder) { - TestCatalog catalog(logger_); - catalog.AddModel(MakeModel("alpha:1", "alpha", 1, "alpha", "/cache/alpha-1")); - catalog.AddModel(MakeModel("alpha:3", "alpha", 3, "alpha")); - catalog.AddModel(MakeModel("alpha:2", "alpha", 2, "alpha", "/cache/alpha-2")); - catalog.AddModel(MakeModel("beta:1", "beta", 1, "beta", "/cache/beta-1")); - - auto cached = catalog.GetCachedModels(); - - ASSERT_EQ(cached.size(), 3u); - EXPECT_EQ(cached[0]->Info().model_id, "alpha:2"); - EXPECT_EQ(cached[1]->Info().model_id, "alpha:1"); - EXPECT_EQ(cached[2]->Info().model_id, "beta:1"); - - for (auto* variant : cached) { - EXPECT_FALSE(variant->IsContainer()); - EXPECT_TRUE(variant->IsCached()); - EXPECT_EQ(variant->Variants().size(), 1u); - EXPECT_EQ(variant, catalog.GetModelVariant(variant->Info().model_id)); - } -} - -TEST_F(BaseModelCatalogTest, GetModelVariant_ById_ReturnsVariantNotContainer) { - TestCatalog catalog(logger_); - catalog.AddModel(MakeModel("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); - catalog.AddModel(MakeModel("phi-3-mini:2", "phi-3-mini", 2, "phi-3")); - - // Looking up by id returns the specific variant - Model* v1 = catalog.GetModelVariant("phi-3-mini:1"); - ASSERT_NE(v1, nullptr); - EXPECT_EQ(v1->Info().model_id, "phi-3-mini:1"); - - Model* v2 = catalog.GetModelVariant("phi-3-mini:2"); - ASSERT_NE(v2, nullptr); - EXPECT_EQ(v2->Info().model_id, "phi-3-mini:2"); - - // Looking up by alias returns the Model container (delegates to selected variant) - Model* container = catalog.GetModel("phi-3"); - ASSERT_NE(container, nullptr); - EXPECT_EQ(container->Variants().size(), 2u); -} - -TEST_F(BaseModelCatalogTest, GetModelVersionsDoesNotIntegrateFetchedVariants) { - QueryingTestCatalog catalog(logger_); - catalog.AddModel(MakeModel("phi-3-mini:2", "phi-3-mini", 2, "phi-3")); - - std::vector version_results; - version_results.push_back(MakeModel("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); - catalog.SetVersionFetchResults(std::move(version_results)); - - auto versions = catalog.GetModelVersions("phi-3", "", 0); - ASSERT_EQ(versions.size(), 1u); - EXPECT_EQ(versions[0]->Info().model_id, "phi-3-mini:1"); - - auto* container = catalog.GetModel("phi-3"); - ASSERT_NE(container, nullptr); - EXPECT_EQ(container->Variants().size(), 1u) - << "GetModelVersions should not add fetched versions to the catalog's main indices."; -} - -TEST_F(BaseModelCatalogTest, GetModelVersionsCrossAliasPointersRemainValid) { - QueryingTestCatalog catalog(logger_); - catalog.AddModel(MakeModel("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); - catalog.AddModel(MakeModel("llama:1", "llama", 1, "llama")); - - // Seed version results for both aliases. - std::vector phi3_versions; - phi3_versions.push_back(MakeModel("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); - phi3_versions.push_back(MakeModel("phi-3-mini:2", "phi-3-mini", 2, "phi-3")); - catalog.SetVersionFetchResults(std::move(phi3_versions)); - - // First query: phi-3 - auto phi3_result = catalog.GetModelVersions("phi-3", "", 0); - ASSERT_EQ(phi3_result.size(), 2u); - Model* phi3_ptr = phi3_result[0]; - - // Second query: llama — must not invalidate phi3_ptr. - std::vector llama_versions; - llama_versions.push_back(MakeModel("llama:1", "llama", 1, "llama")); - llama_versions.push_back(MakeModel("llama:2", "llama", 2, "llama")); - catalog.SetVersionFetchResults(std::move(llama_versions)); - - auto llama_result = catalog.GetModelVersions("llama", "", 0); - ASSERT_EQ(llama_result.size(), 2u); - - // phi3_ptr must still be alive and accessible. - EXPECT_EQ(phi3_ptr->Info().alias, "phi-3") - << "Querying a different alias should not invalidate pointers from a prior GetModelVersions call."; -} - - TEST_F(BaseModelCatalogTest, GetModelVersionsMaxVersionsSelectsLatestRegardlessOfFetchOrder) { - QueryingTestCatalog catalog(logger_); - - std::vector version_results; - // Intentionally unsorted fetch order: v2, v1, v3. - version_results.push_back(MakeModel("phi-3-mini-generic-cpu:2", "phi-3-mini", 2, "phi-3")); - version_results.push_back(MakeModel("phi-3-mini-generic-cpu:1", "phi-3-mini", 1, "phi-3")); - version_results.push_back(MakeModel("phi-3-mini-generic-cpu:3", "phi-3-mini", 3, "phi-3")); - catalog.SetVersionFetchResults(std::move(version_results)); - - auto versions = catalog.GetModelVersions("phi-3", "", /*max_versions=*/1); - ASSERT_EQ(versions.size(), 1u); - EXPECT_EQ(versions.front()->Info().version, 3) - << "max_versions=1 should pick the latest version even when fetch order is arbitrary."; - } - -TEST_F(BaseModelCatalogTest, GetModelVariantIdIntegratesFetchedVariant) { - QueryingTestCatalog catalog(logger_); - catalog.AddModel(MakeModel("phi-3-mini:2", "phi-3-mini", 2, "phi-3")); - - std::vector id_results; - id_results.push_back(MakeModel("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); - catalog.SetIdFetchResults(std::move(id_results)); - - auto* fetched = catalog.GetModelVariant("phi-3-mini:1"); - ASSERT_NE(fetched, nullptr); - EXPECT_EQ(fetched->Info().model_id, "phi-3-mini:1"); - - auto* container = catalog.GetModel("phi-3"); - ASSERT_NE(container, nullptr); - EXPECT_EQ(container->Variants().size(), 2u) - << "ID-based fetches should still integrate so download-specific lookups persist in the catalog."; -} - -TEST_F(BaseModelCatalogTest, GetModelVariantIdIntegrationPreservesPriorityOrdering) { - QueryingTestCatalog catalog(logger_); - catalog.AddModel(MakeModel("phi-3-mini-generic-cpu:1", "phi-3-mini", 1, "phi-3")); - - std::vector id_results; - id_results.push_back(MakeModel("phi-3-mini-npu:1", "phi-3-mini", 1, "phi-3")); - catalog.SetIdFetchResults(std::move(id_results)); - - auto* fetched = catalog.GetModelVariant("phi-3-mini-npu:1"); - ASSERT_NE(fetched, nullptr); - - auto* container = catalog.GetModel("phi-3"); - ASSERT_NE(container, nullptr); - auto variants = container->Variants(); - ASSERT_EQ(variants.size(), 2u); - EXPECT_EQ(variants.front()->Info().model_id, "phi-3-mini-npu:1") - << "Integrated variants should be re-sorted so higher-priority devices stay first."; -} diff --git a/sdk_v2/cpp/test/internal_api/chat/chat_session_test.cc b/sdk_v2/cpp/test/internal_api/chat/chat_session_test.cc index 281f8198a..fb4c53f72 100644 --- a/sdk_v2/cpp/test/internal_api/chat/chat_session_test.cc +++ b/sdk_v2/cpp/test/internal_api/chat/chat_session_test.cc @@ -70,7 +70,7 @@ class ChatSessionTest : public ::testing::Test { static inline Model catalog_model_ = [] { ModelInfo info; info.task = "chat-completion"; - return Model::FromModelInfo(std::move(info), "", svc_.download_manager, svc_.model_load_manager); + return Model::FromModelInfo(std::move(info), svc_.download_manager, svc_.model_load_manager); }(); TelemetryLogger null_telemetry_{"test", fl::test::NullLog()}; fl::test::NullSessionManager null_session_manager_; diff --git a/sdk_v2/cpp/test/internal_api/model_catalog_test.cc b/sdk_v2/cpp/test/internal_api/model_catalog_test.cc new file mode 100644 index 000000000..9cea021ff --- /dev/null +++ b/sdk_v2/cpp/test/internal_api/model_catalog_test.cc @@ -0,0 +1,498 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +// +// Tests for ModelCatalog — the aggregating store behind ICatalog. Fetch lives in +// IModelSources (here a FakeModelSource); the store owns the ModelFactory, groups variants +// by alias, keeps same-model_id shadows from different sources, and serves a preferred-only +// public view (UniqueVariants). Also covers source preference, SelectDefaultVariant +// precedence, the Unregister / RemoveVariant fallback path, and catalog_source JSON round-trip. +// +#include "catalog/model_catalog.h" +#include "catalog/model_source.h" +#include "internal_api/test_helpers.h" +#include "logger.h" +#include "model.h" +#include "model_info.h" + +#include +#include + +#include +#include +#include +#include +#include + +using namespace fl; + +namespace { + +ModelInfo MakeInfo(const std::string& model_id, const std::string& name, int version, + const std::string& alias, const std::string& local_path = {}, + CatalogSource source = CatalogSource::kPublic) { + ModelInfo info; + info.model_id = model_id; + info.name = name; + info.version = version; + info.alias = alias; + info.local_path = local_path; + info.catalog_source = source; + return info; +} + +// A fetch-only IModelSource returning canned ModelInfo. Configure it before the first query. +class FakeModelSource : public IModelSource { + public: + explicit FakeModelSource(CatalogSource source = CatalogSource::kPublic, std::string name = "fake-source") + : source_(source), name_(std::move(name)) {} + + CatalogSource Source() const override { return source_; } + std::string Name() const override { return name_; } + + std::vector FetchModels() const override { return models_; } + + std::vector FetchModelVersions(const std::string& model_alias, + const std::string& model_name = "") const override { + std::vector result; + for (const auto& info : version_results_) { + if (info.alias != model_alias) { + continue; + } + if (!model_name.empty() && info.name != model_name) { + continue; + } + result.push_back(info); + } + return result; + } + + std::vector FetchModelsByIds(const std::vector& model_ids) const override { + const std::unordered_set requested(model_ids.begin(), model_ids.end()); + std::vector result; + for (const auto& info : id_results_) { + if (requested.contains(info.model_id)) { + result.push_back(info); + } + } + return result; + } + + void AddModel(ModelInfo info) { models_.push_back(std::move(info)); } + void SetVersionResults(std::vector results) { version_results_ = std::move(results); } + void SetIdResults(std::vector results) { id_results_ = std::move(results); } + + private: + CatalogSource source_; + std::string name_; + std::vector models_; + std::vector version_results_; + std::vector id_results_; +}; + +Model* FindVariant(const std::vector& variants, const std::string& model_id) { + for (auto* v : variants) { + if (v->Info().model_id == model_id) { + return v; + } + } + return nullptr; +} + +} // namespace + +// ======================================================================== +// Fixture — a default public FakeModelSource plus optional extra sources. +// ======================================================================== + +class ModelCatalogTest : public ::testing::Test { + protected: + ModelCatalogTest() { default_source_ = AddSource(CatalogSource::kPublic); } + + FakeModelSource* AddSource(CatalogSource source, std::string name = "fake-source") { + auto s = std::make_unique(source, std::move(name)); + auto* ptr = s.get(); + pending_sources_.push_back(std::move(s)); + return ptr; + } + + void AddModel(ModelInfo info) { default_source_->AddModel(std::move(info)); } + + ModelCatalog& Catalog(const std::string& name = "test-catalog") { + if (!catalog_) { + catalog_ = std::make_unique( + name, std::move(pending_sources_), + [this](ModelInfo info) { + return Model::FromModelInfo(std::move(info), svc_.download_manager, svc_.model_load_manager); + }, + logger_); + } + return *catalog_; + } + + StderrLogger logger_; + fl::test::FakeServiceBindings svc_; + std::vector> pending_sources_; + FakeModelSource* default_source_ = nullptr; + std::unique_ptr catalog_; +}; + +// ======================================================================== +// GetName +// ======================================================================== + +TEST_F(ModelCatalogTest, GetName_ReturnsNameFromConstruction) { + EXPECT_EQ(Catalog("test-catalog").GetName(), "test-catalog"); +} + +// ======================================================================== +// GetModel +// ======================================================================== + +TEST_F(ModelCatalogTest, GetModel_ByName_ReturnsNullptr) { + AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); + EXPECT_EQ(Catalog().GetModel("phi-3-mini"), nullptr); +} + +TEST_F(ModelCatalogTest, GetModel_ByAlias) { + AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); + Model* m = Catalog().GetModel("phi-3"); + ASSERT_NE(m, nullptr); + EXPECT_EQ(m->Info().model_id, "phi-3-mini:1"); +} + +TEST_F(ModelCatalogTest, GetModel_Nonexistent_ReturnsNullptr) { + AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); + EXPECT_EQ(Catalog().GetModel("nonexistent"), nullptr); +} + +TEST_F(ModelCatalogTest, GetModel_EmptyCatalog_ReturnsNullptr) { + EXPECT_EQ(Catalog().GetModel("anything"), nullptr); +} + +// ======================================================================== +// GetModelVariant +// ======================================================================== + +TEST_F(ModelCatalogTest, GetModelVariant_ById) { + AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); + Model* m = Catalog().GetModelVariant("phi-3-mini:1"); + ASSERT_NE(m, nullptr); + EXPECT_EQ(m->Info().model_id, "phi-3-mini:1"); +} + +TEST_F(ModelCatalogTest, GetModelVariant_Nonexistent_ReturnsNullptr) { + AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); + EXPECT_EQ(Catalog().GetModelVariant("nonexistent"), nullptr); +} + +TEST_F(ModelCatalogTest, GetModel_VariantsAccessible) { + AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); + AddModel(MakeInfo("phi-3-mini:2", "phi-3-mini", 2, "phi-3")); + Model* container = Catalog().GetModel("phi-3"); + ASSERT_NE(container, nullptr); + EXPECT_EQ(container->Variants().size(), 2u); +} + +TEST_F(ModelCatalogTest, GetModel_EmptyString_ReturnsNullptr) { + AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); + EXPECT_EQ(Catalog().GetModel(""), nullptr); +} + +// ======================================================================== +// ListModels +// ======================================================================== + +TEST_F(ModelCatalogTest, ListModels_ReturnsGroupedByAlias) { + AddModel(MakeInfo("a:1", "a", 1, "a")); + AddModel(MakeInfo("b:1", "b", 1, "b")); + EXPECT_EQ(Catalog().ListModels().size(), 2u); +} + +TEST_F(ModelCatalogTest, ListModels_VariantsGroupedIntoSingleModel) { + AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); + AddModel(MakeInfo("phi-3-mini:2", "phi-3-mini", 2, "phi-3")); + auto list = Catalog().ListModels(); + ASSERT_EQ(list.size(), 1u); + EXPECT_EQ(list[0]->Alias(), "phi-3"); + EXPECT_EQ(list[0]->Variants().size(), 2u); +} + +TEST_F(ModelCatalogTest, ListModels_MultipleAliasGroups) { + AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); + AddModel(MakeInfo("phi-3-mini:2", "phi-3-mini", 2, "phi-3")); + AddModel(MakeInfo("llama:1", "llama", 1, "llama")); + EXPECT_EQ(Catalog().ListModels().size(), 2u); +} + +// ======================================================================== +// Invalid entries are skipped +// ======================================================================== + +TEST_F(ModelCatalogTest, GetModel_SkipsInvalidEntries) { + AddModel(MakeInfo("", "bad", 0, "bad-alias")); // missing model_id + AddModel(MakeInfo("good:1", "good", 1, "good-alias")); + + auto& catalog = Catalog(); + EXPECT_EQ(catalog.ListModels().size(), 1u); + EXPECT_NE(catalog.GetModelVariant("good:1"), nullptr); + EXPECT_EQ(catalog.GetModel("bad"), nullptr); +} + +// ======================================================================== +// Cached-variant preference +// ======================================================================== + +TEST_F(ModelCatalogTest, GroupedModel_PrefersCachedVariant) { + AddModel(MakeInfo("phi-3:1", "phi-3", 1, "phi-3")); + AddModel(MakeInfo("phi-3:2", "phi-3", 2, "phi-3", "/path/to/cached/model")); + + auto* m = Catalog().GetModel("phi-3"); + ASSERT_NE(m, nullptr); + EXPECT_TRUE(m->IsCached()); + EXPECT_EQ(m->Info().model_id, "phi-3:2"); +} + +TEST_F(ModelCatalogTest, GetCachedModelsReturnsEveryCachedLeafInCatalogVariantOrder) { + AddModel(MakeInfo("alpha:1", "alpha", 1, "alpha", "/cache/alpha-1")); + AddModel(MakeInfo("alpha:3", "alpha", 3, "alpha")); + AddModel(MakeInfo("alpha:2", "alpha", 2, "alpha", "/cache/alpha-2")); + AddModel(MakeInfo("beta:1", "beta", 1, "beta", "/cache/beta-1")); + + auto& catalog = Catalog(); + auto cached = catalog.GetCachedModels(); + + ASSERT_EQ(cached.size(), 3u); + EXPECT_EQ(cached[0]->Info().model_id, "alpha:2"); + EXPECT_EQ(cached[1]->Info().model_id, "alpha:1"); + EXPECT_EQ(cached[2]->Info().model_id, "beta:1"); + + for (auto* variant : cached) { + EXPECT_FALSE(variant->IsContainer()); + EXPECT_TRUE(variant->IsCached()); + EXPECT_EQ(variant, catalog.GetModelVariant(variant->Info().model_id)); + } +} + +TEST_F(ModelCatalogTest, GetModelVariant_ById_ReturnsVariantNotContainer) { + AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); + AddModel(MakeInfo("phi-3-mini:2", "phi-3-mini", 2, "phi-3")); + + auto& catalog = Catalog(); + Model* v1 = catalog.GetModelVariant("phi-3-mini:1"); + ASSERT_NE(v1, nullptr); + EXPECT_EQ(v1->Info().model_id, "phi-3-mini:1"); + + Model* v2 = catalog.GetModelVariant("phi-3-mini:2"); + ASSERT_NE(v2, nullptr); + EXPECT_EQ(v2->Info().model_id, "phi-3-mini:2"); + + Model* container = catalog.GetModel("phi-3"); + ASSERT_NE(container, nullptr); + EXPECT_EQ(container->Variants().size(), 2u); +} + +// ======================================================================== +// GetModelVersions / by-id fetch (delegates to sources) +// ======================================================================== + +TEST_F(ModelCatalogTest, GetModelVersionsDoesNotIntegrateFetchedVariants) { + AddModel(MakeInfo("phi-3-mini:2", "phi-3-mini", 2, "phi-3")); + default_source_->SetVersionResults({MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")}); + + auto& catalog = Catalog(); + auto versions = catalog.GetModelVersions("phi-3", "", 0); + ASSERT_EQ(versions.size(), 1u); + EXPECT_EQ(versions[0]->Info().model_id, "phi-3-mini:1"); + + auto* container = catalog.GetModel("phi-3"); + ASSERT_NE(container, nullptr); + EXPECT_EQ(container->Variants().size(), 1u) + << "GetModelVersions should not add fetched versions to the catalog's main indices."; +} + +TEST_F(ModelCatalogTest, GetModelVersionsCrossAliasPointersRemainValid) { + AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); + AddModel(MakeInfo("llama:1", "llama", 1, "llama")); + + default_source_->SetVersionResults({ + MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3"), + MakeInfo("phi-3-mini:2", "phi-3-mini", 2, "phi-3"), + MakeInfo("llama:1", "llama", 1, "llama"), + MakeInfo("llama:2", "llama", 2, "llama"), + }); + + auto& catalog = Catalog(); + auto phi3_result = catalog.GetModelVersions("phi-3", "", 0); + ASSERT_EQ(phi3_result.size(), 2u); + Model* phi3_ptr = phi3_result[0]; + + auto llama_result = catalog.GetModelVersions("llama", "", 0); + ASSERT_EQ(llama_result.size(), 2u); + + EXPECT_EQ(phi3_ptr->Info().alias, "phi-3") + << "Querying a different alias should not invalidate pointers from a prior GetModelVersions call."; +} + +TEST_F(ModelCatalogTest, GetModelVersionsMaxVersionsSelectsLatestRegardlessOfFetchOrder) { + default_source_->SetVersionResults({ + MakeInfo("phi-3-mini-generic-cpu:2", "phi-3-mini", 2, "phi-3"), + MakeInfo("phi-3-mini-generic-cpu:1", "phi-3-mini", 1, "phi-3"), + MakeInfo("phi-3-mini-generic-cpu:3", "phi-3-mini", 3, "phi-3"), + }); + + auto versions = Catalog().GetModelVersions("phi-3", "", /*max_versions=*/1); + ASSERT_EQ(versions.size(), 1u); + EXPECT_EQ(versions.front()->Info().version, 3) + << "max_versions=1 should pick the latest version even when fetch order is arbitrary."; +} + +TEST_F(ModelCatalogTest, GetModelVariantIdIntegratesFetchedVariant) { + AddModel(MakeInfo("phi-3-mini:2", "phi-3-mini", 2, "phi-3")); + default_source_->SetIdResults({MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")}); + + auto& catalog = Catalog(); + auto* fetched = catalog.GetModelVariant("phi-3-mini:1"); + ASSERT_NE(fetched, nullptr); + EXPECT_EQ(fetched->Info().model_id, "phi-3-mini:1"); + + auto* container = catalog.GetModel("phi-3"); + ASSERT_NE(container, nullptr); + EXPECT_EQ(container->Variants().size(), 2u) + << "ID-based fetches should still integrate so download-specific lookups persist in the catalog."; +} + +TEST_F(ModelCatalogTest, GetModelVariantIdIntegrationPreservesPriorityOrdering) { + AddModel(MakeInfo("phi-3-mini-generic-cpu:1", "phi-3-mini", 1, "phi-3")); + default_source_->SetIdResults({MakeInfo("phi-3-mini-npu:1", "phi-3-mini", 1, "phi-3")}); + + auto& catalog = Catalog(); + auto* fetched = catalog.GetModelVariant("phi-3-mini-npu:1"); + ASSERT_NE(fetched, nullptr); + + auto* container = catalog.GetModel("phi-3"); + ASSERT_NE(container, nullptr); + auto variants = container->Variants(); + ASSERT_EQ(variants.size(), 2u); + EXPECT_EQ(variants.front()->Info().model_id, "phi-3-mini-npu:1") + << "Integrated variants should be re-sorted so higher-priority devices stay first."; +} + +// ======================================================================== +// Multi-source: union of aliases, shadow variants, preference, visibility +// ======================================================================== + +TEST_F(ModelCatalogTest, MultipleSourcesUnionAliases) { + default_source_->AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); + auto* local = AddSource(CatalogSource::kLocal, "local-source"); + local->AddModel(MakeInfo("llama:1", "llama", 1, "llama", "/cache/llama", CatalogSource::kLocal)); + + auto list = Catalog().ListModels(); + EXPECT_EQ(list.size(), 2u); +} + +TEST_F(ModelCatalogTest, ShadowVariantsKeptInternallyButHiddenFromUniqueView) { + // Same model_id served by public and local sources → shadow variants. + default_source_->AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); + auto* local = AddSource(CatalogSource::kLocal, "local-source"); + local->AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3", "/cache/phi", CatalogSource::kLocal)); + + auto* container = Catalog().GetModel("phi-3"); + ASSERT_NE(container, nullptr); + + // Internal enumeration keeps both shadows. + EXPECT_EQ(container->Variants().size(), 2u); + // Public view collapses to one leaf per model_id. + auto unique = container->UniqueVariants(); + ASSERT_EQ(unique.size(), 1u); + EXPECT_EQ(unique.front()->Info().catalog_source, CatalogSource::kLocal) + << "The preferred (local) shadow should be the visible copy."; +} + +TEST_F(ModelCatalogTest, IdIndexResolvesToPreferredSource) { + default_source_->AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); + auto* priv = AddSource(CatalogSource::kPrivate, "private-source"); + priv->AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3", "", CatalogSource::kPrivate)); + + auto* variant = Catalog().GetModelVariant("phi-3-mini:1"); + ASSERT_NE(variant, nullptr); + EXPECT_EQ(variant->Info().catalog_source, CatalogSource::kPrivate) + << "private ranks above public, so by-id lookup resolves to the private shadow."; +} + +TEST_F(ModelCatalogTest, SelectDefaultVariantCachedBeatsSourcePreference) { + // Preferred-source (private) copy is uncached; lower-priority (public) copy is cached. + default_source_->AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3", "/cache/public")); + auto* priv = AddSource(CatalogSource::kPrivate, "private-source"); + priv->AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3", "", CatalogSource::kPrivate)); + + auto* container = Catalog().GetModel("phi-3"); + ASSERT_NE(container, nullptr); + EXPECT_TRUE(container->IsCached()); + EXPECT_EQ(container->Info().catalog_source, CatalogSource::kPublic) + << "cached beats source preference for the container's default selection."; +} + +// ======================================================================== +// Unregister / RemoveVariant fallback +// ======================================================================== + +TEST_F(ModelCatalogTest, UnregisterRemovesShadowAndReSelectsSurvivor) { + default_source_->AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); + auto* local = AddSource(CatalogSource::kLocal, "local-source"); + local->AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3", "/cache/phi", CatalogSource::kLocal)); + + auto& catalog = catalog_ ? *catalog_ : Catalog(); + auto* container = catalog.GetModel("phi-3"); + ASSERT_NE(container, nullptr); + ASSERT_EQ(container->Variants().size(), 2u); + + // The by-id lookup and the local shadow both resolve to the local (preferred) copy. + EXPECT_EQ(catalog.GetModelVariant("phi-3-mini:1")->Info().catalog_source, CatalogSource::kLocal); + + catalog.Unregister("phi-3-mini:1"); + + // The local shadow is gone; the surviving public copy is now the by-id result. + auto* survivor = catalog.GetModelVariant("phi-3-mini:1"); + ASSERT_NE(survivor, nullptr); + EXPECT_EQ(survivor->Info().catalog_source, CatalogSource::kPublic); + EXPECT_EQ(container->Variants().size(), 1u); +} + +TEST_F(ModelCatalogTest, UnregisterLastVariantRemovesContainer) { + AddModel(MakeInfo("solo:1", "solo", 1, "solo", "/cache/solo", CatalogSource::kLocal)); + + auto& catalog = Catalog(); + ASSERT_NE(catalog.GetModel("solo"), nullptr); + + catalog.Unregister("solo:1"); + + EXPECT_EQ(catalog.GetModel("solo"), nullptr); + EXPECT_EQ(catalog.GetModelVariant("solo:1"), nullptr); + EXPECT_TRUE(catalog.ListModels().empty()); +} + +TEST_F(ModelCatalogTest, UnregisterUnknownIdThrows) { + AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); + auto& catalog = Catalog(); + catalog.ListModels(); // force populate + EXPECT_THROW(catalog.Unregister("does-not-exist:1"), std::exception); +} + +// ======================================================================== +// catalog_source round-trips through the cache JSON +// ======================================================================== + +TEST_F(ModelCatalogTest, CatalogSourceRoundTripsThroughJson) { + for (auto source : {CatalogSource::kPublic, CatalogSource::kPrivate, CatalogSource::kLocal}) { + ModelInfo info = MakeInfo("m:1", "m", 1, "m"); + info.catalog_source = source; + const auto restored = ModelInfoFromJson(ModelInfoToJson(info)); + EXPECT_EQ(restored.catalog_source, source); + } +} + +TEST_F(ModelCatalogTest, CatalogSourceAbsentFromJsonDecodesAsPublic) { + nlohmann::json j = ModelInfoToJson(MakeInfo("m:1", "m", 1, "m")); + EXPECT_FALSE(j.contains("catalogSource")) << "public is the default and is not serialized."; + EXPECT_EQ(ModelInfoFromJson(j).catalog_source, CatalogSource::kPublic); +} diff --git a/sdk_v2/cpp/test/internal_api/model_io_info_test.cc b/sdk_v2/cpp/test/internal_api/model_io_info_test.cc index 4ab731ee5..18f23d7d9 100644 --- a/sdk_v2/cpp/test/internal_api/model_io_info_test.cc +++ b/sdk_v2/cpp/test/internal_api/model_io_info_test.cc @@ -24,7 +24,7 @@ static Model MakeModelWithTask(const std::string& task) { info.version = 1; info.alias = "test-alias"; info.task = task; - return Model::FromModelInfo(std::move(info), "", + return Model::FromModelInfo(std::move(info), svc.download_manager, svc.model_load_manager); } diff --git a/sdk_v2/cpp/test/internal_api/model_sorting_test.cc b/sdk_v2/cpp/test/internal_api/model_sorting_test.cc index 73dfb5831..b13b152d2 100644 --- a/sdk_v2/cpp/test/internal_api/model_sorting_test.cc +++ b/sdk_v2/cpp/test/internal_api/model_sorting_test.cc @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. // -// Tests for model variant sorting within BaseModelCatalog. +// Tests for model variant sorting within ModelCatalog. // Verifies the C++ port of C# AzureFoundryService.SortModels / // CompareModelsForSort / GetModelPriority: // - Device-type priority: NPU > vendor-GPU > CUDA-GPU > generic-GPU @@ -9,7 +9,8 @@ // - Version descending (higher first) // - CreatedAtUnix descending (newer first) // -#include "catalog/base_model_catalog.h" +#include "catalog/model_catalog.h" +#include "catalog/model_source.h" #include "internal_api/test_helpers.h" #include "logger.h" #include "model.h" @@ -18,38 +19,70 @@ #include #include +#include #include +#include #include using namespace fl; +namespace { + +// Minimal fetch-only source returning canned ModelInfo for the sort harness. +class FakeSortSource : public IModelSource { + public: + CatalogSource Source() const override { return CatalogSource::kPublic; } + std::string Name() const override { return "sort-fake-source"; } + std::vector FetchModels() const override { return models_; } + void AddModel(ModelInfo info) { models_.push_back(std::move(info)); } + + private: + std::vector models_; +}; + +} // namespace + // ======================================================================== -// Concrete test catalog — same pattern as base_model_catalog_test.cc +// Thin wrapper around ModelCatalog preserving the old test ergonomics: +// AddModel(ModelInfo) before querying, then GetModel(alias). // ======================================================================== -class SortTestCatalog : public BaseModelCatalog { +class SortTestCatalog { public: - explicit SortTestCatalog(ILogger& logger) : BaseModelCatalog("sort-test-catalog", logger) {} - - void AddModel(Model model) { - models_.push_back(std::move(model)); + explicit SortTestCatalog(ILogger& logger) : logger_(logger) { + auto source = std::make_unique(); + source_ = source.get(); + sources_.push_back(std::move(source)); } - protected: - std::vector FetchModels() const override { - return std::move(models_); + void AddModel(ModelInfo info) { source_->AddModel(std::move(info)); } + + Model* GetModel(const std::string& alias) { + if (!catalog_) { + catalog_ = std::make_unique( + "sort-test-catalog", std::move(sources_), + [this](ModelInfo info) { + return Model::FromModelInfo(std::move(info), svc_.download_manager, svc_.model_load_manager); + }, + logger_); + } + return catalog_->GetModel(alias); } private: - mutable std::vector models_; + ILogger& logger_; + fl::test::FakeServiceBindings svc_; + std::vector> sources_; + FakeSortSource* source_ = nullptr; + std::unique_ptr catalog_; }; -// Helper: create a Model with device suffix baked into model_id. -static Model MakeModel(const std::string& base_name, - const std::string& device_suffix, - int version, - const std::string& alias, - int64_t created_at_unix = 0) { +// Helper: create a ModelInfo with device suffix baked into model_id. +static ModelInfo MakeModel(const std::string& base_name, + const std::string& device_suffix, + int version, + const std::string& alias, + int64_t created_at_unix = 0) { ModelInfo info; info.model_id = base_name + device_suffix + ":" + std::to_string(version); info.name = base_name; @@ -60,9 +93,7 @@ static Model MakeModel(const std::string& base_name, info.int_properties[FOUNDRY_LOCAL_MODEL_PROP_CREATED_AT_UNIX_INT] = created_at_unix; } - static fl::test::FakeServiceBindings svc; - return Model::FromModelInfo(std::move(info), "", - svc.download_manager, svc.model_load_manager); + return info; } // ======================================================================== diff --git a/sdk_v2/cpp/test/internal_api/session_manager_test.cc b/sdk_v2/cpp/test/internal_api/session_manager_test.cc index b7b71ea5c..d1b8b0f83 100644 --- a/sdk_v2/cpp/test/internal_api/session_manager_test.cc +++ b/sdk_v2/cpp/test/internal_api/session_manager_test.cc @@ -86,7 +86,7 @@ class SessionManagerTest : public ::testing::Test { static inline GenAIModelInstance* model_ = nullptr; static inline fl::test::FakeServiceBindings svc_; static inline Model catalog_model_ = Model::FromModelInfo( - ModelInfo{}, "", svc_.download_manager, svc_.model_load_manager); + ModelInfo{}, svc_.download_manager, svc_.model_load_manager); TelemetryLogger null_telemetry_{"test", fl::test::NullLog()}; }; @@ -373,7 +373,7 @@ bool WaitUntil(Pred pred, std::chrono::milliseconds timeout) { TEST(SessionManagerCancelTest, CancelAllCancelsInFlightRequestsOnEverySession) { fl::test::FakeServiceBindings svc; - Model catalog_model = Model::FromModelInfo(ModelInfo{}, "", svc.download_manager, svc.model_load_manager); + Model catalog_model = Model::FromModelInfo(ModelInfo{}, svc.download_manager, svc.model_load_manager); TelemetryLogger telemetry{"test", fl::test::NullLog()}; SessionManager mgr(fl::test::NullLog()); @@ -414,7 +414,7 @@ TEST(SessionManagerCancelTest, RequestAdmittedAfterCancelIsStampedCanceled) { // can't see this request yet, so the latch must stamp it on insert — otherwise it would run a full // uncanceled turn and block JoinAll() until the 5s safety deadline. fl::test::FakeServiceBindings svc; - Model catalog_model = Model::FromModelInfo(ModelInfo{}, "", svc.download_manager, svc.model_load_manager); + Model catalog_model = Model::FromModelInfo(ModelInfo{}, svc.download_manager, svc.model_load_manager); TelemetryLogger telemetry{"test", fl::test::NullLog()}; SessionManager mgr(fl::test::NullLog()); diff --git a/sdk_v2/cpp/test/internal_api/test_helpers.h b/sdk_v2/cpp/test/internal_api/test_helpers.h index d53677dd8..bb01798e1 100644 --- a/sdk_v2/cpp/test/internal_api/test_helpers.h +++ b/sdk_v2/cpp/test/internal_api/test_helpers.h @@ -45,7 +45,7 @@ inline ILogger& NullLog() { /// One-stop bag of cheap fakes for tests that need to construct a leaf `Model` via /// `FromModelInfo` but don't exercise Download/Load. Public fields by design — no invariants /// to protect, and the field names match the matching `FromModelInfo` parameter names so the -/// call site reads as `FromModelInfo(info, "", svc.download_manager, svc.model_load_manager)`. +/// call site reads as `FromModelInfo(info, svc.download_manager, svc.model_load_manager)`. struct FakeServiceBindings { CpuOnlyEpDetector ep_detector; NullLogger logger; diff --git a/sdk_v2/cpp/test/internal_api/web_service_test.cc b/sdk_v2/cpp/test/internal_api/web_service_test.cc index 97389f18e..b43f2660d 100644 --- a/sdk_v2/cpp/test/internal_api/web_service_test.cc +++ b/sdk_v2/cpp/test/internal_api/web_service_test.cc @@ -75,18 +75,19 @@ class WebServiceTest : public ::testing::Test { // Populate with test models catalog_->AddModel(Model::FromModelInfo( - test::MakeTestModelInfo("alpha-model", "acme-corp"), "", + test::MakeTestModelInfo("alpha-model", "acme-corp"), svc_.download_manager, svc_.model_load_manager)); catalog_->AddModel(Model::FromModelInfo( - test::MakeTestModelInfo("beta-model", "contoso"), "", + test::MakeTestModelInfo("beta-model", "contoso"), svc_.download_manager, svc_.model_load_manager)); const auto loadable_model_path = test::GetTestDataModelPath(test::kLoadableTestModelAlias); ASSERT_TRUE(std::filesystem::exists(loadable_model_path)) << "Expected loadable test model at " << loadable_model_path; + ModelInfo loadable_info = test::MakeTestModelInfo(test::kLoadableTestModelAlias, "microsoft"); + loadable_info.local_path = loadable_model_path; catalog_->AddModel(Model::FromModelInfo( - test::MakeTestModelInfo(test::kLoadableTestModelAlias, "microsoft"), - loadable_model_path, + std::move(loadable_info), svc_.download_manager, *model_load_manager_)); From c42dd753c2072b85d055136dcdfed4363b2847fb Mon Sep 17 00:00:00 2001 From: Scott McKay Date: Mon, 17 Aug 2026 09:39:21 +1000 Subject: [PATCH 3/4] Fix build --- sdk_v2/cpp/test/internal_api/model_catalog_test.cc | 9 --------- 1 file changed, 9 deletions(-) diff --git a/sdk_v2/cpp/test/internal_api/model_catalog_test.cc b/sdk_v2/cpp/test/internal_api/model_catalog_test.cc index 9cea021ff..f79f087ee 100644 --- a/sdk_v2/cpp/test/internal_api/model_catalog_test.cc +++ b/sdk_v2/cpp/test/internal_api/model_catalog_test.cc @@ -89,15 +89,6 @@ class FakeModelSource : public IModelSource { std::vector id_results_; }; -Model* FindVariant(const std::vector& variants, const std::string& model_id) { - for (auto* v : variants) { - if (v->Info().model_id == model_id) { - return v; - } - } - return nullptr; -} - } // namespace // ======================================================================== From 5247aaec5f22a8c951be9c1e0de1927339a134b8 Mon Sep 17 00:00:00 2001 From: Scott McKay Date: Wed, 19 Aug 2026 15:12:16 +1000 Subject: [PATCH 4/4] Simplify --- sdk_v2/cpp/docs/MultiCatalogSupportPlan.md | 219 +++++------------- sdk_v2/cpp/src/c_api.cc | 4 +- sdk_v2/cpp/src/catalog/model_catalog.h | 2 +- sdk_v2/cpp/src/model.cc | 23 -- sdk_v2/cpp/src/model.h | 7 - sdk_v2/cpp/src/service/models_handlers.cc | 5 +- sdk_v2/cpp/test/internal_api/c_api_test.cc | 76 ++++++ .../test/internal_api/model_catalog_test.cc | 54 +++-- .../cpp/test/internal_api/web_service_test.cc | 33 +++ 9 files changed, 209 insertions(+), 214 deletions(-) diff --git a/sdk_v2/cpp/docs/MultiCatalogSupportPlan.md b/sdk_v2/cpp/docs/MultiCatalogSupportPlan.md index 4333de5ed..312d5fe86 100644 --- a/sdk_v2/cpp/docs/MultiCatalogSupportPlan.md +++ b/sdk_v2/cpp/docs/MultiCatalogSupportPlan.md @@ -2,8 +2,6 @@ > Status: **Proposal for review** > Scope: `sdk_v2/cpp` catalog subsystem -> Supersedes: `MultiCatalogAggregationPlan.md` and `MultiSourceCatalogDesign.md` -> (kept for history; do not edit) ## Summary @@ -26,14 +24,16 @@ Split the two responsibilities that `BaseModelCatalog` currently couples: **pure fetchers that return `ModelInfo`** — they do not create `Model` instances. - **Store / query / index / create** lives in a single `ModelCatalog : ICatalog` that owns the `ModelFactory` and all `Model` instances, merges across sources, keeps shadow duplicates, and - serves a filtered (preferred-only) public API view. + serves deterministic enumeration of all variants. On a duplicate (same `model_id` across catalog sources) we keep every copy internally as **shadow -variants**; the **public API view** surfaces only the **preferred** one. Preference is by catalog -source: `local > private > public`. Retaining the shadows enables **fallback** — e.g. unregistering -a BYOM local model that shadows a cloud id re-selects the surviving cloud copy. (Genuine cross-source -duplicates only arise once Private or BYOM lands — see *What "local" means* below — so this machinery -is foundational in the initial scope.) +variants**, and public/global enumeration surfaces every copy in deterministic catalog sort order, +including duplicate `model_id`s. Preference is by catalog source: `local > private > public`, and +governs direct `model_id` lookup and default selection rather than filtering enumeration. Retaining +the shadows enables **fallback** — e.g. unregistering a BYOM local model that shadows a cloud id +re-selects the surviving cloud copy. (Genuine cross-source duplicates only arise once Private or +BYOM lands — see *What "local" means* below — so this machinery is foundational in the initial +scope.) ## What "local" means here @@ -57,24 +57,24 @@ while there are no genuine shadows. Consequence for the initial scope: public ids are unique and `kLocal` stubs are orphans (disjoint from public by construction), so **the single Public source produces no genuine cross-source -duplicates**. The shadow-variant / source-preference / `UniqueVariants` machinery is therefore -foundational — first exercised when Private or BYOM introduces a second source that can serve the same -`model_id`. +duplicates**. The shadow-variant / source-preference / duplicate-preserving enumeration machinery is +therefore foundational — first exercised when Private or BYOM introduces a second source that can +serve the same `model_id`. ## Why this shape - **Source/store split; sources return `ModelInfo`.** The store owns the `ModelFactory`, so sources stay pure fetchers and never touch `DownloadManager` / `ModelLoadManager`. -- **Duplicates are shadow variants, filtered by the public view.** There is no internal - "no duplicates" rule — the user drives selection, so the visible `flModelList` is just a filtered - view, and same-`model_id` copies from different sources may carry different metadata. +- **Duplicates are shadow variants and remain visible in enumeration.** There is no internal or + public "no duplicates" rule: same-`model_id` copies from different sources may carry different + metadata, and callers can inspect every copy in deterministic catalog sort order. Two behavior changes fall out of this: -1. **Container `variants_` may hold same-`model_id` shadows** (today they're distinct). Filtering - moves to the visible view; the internal **`id_index`** (the `model_id → Model*` lookup that backs - by-id queries, built by `RebuildIndex`) resolves each `model_id` to its **preferred** leaf via - source-aware ordering. +1. **Container `variants_` may hold same-`model_id` shadows** (today they're distinct). Public/global + enumeration returns those shadows in deterministic source-aware catalog order; the internal + **`id_index`** (the `model_id → Model*` lookup that backs by-id queries, built by `RebuildIndex`) + resolves each `model_id` to its **preferred** leaf. 2. **`Model*` stays stable for cloud models** (append-only; `RemoveFromCache` only un-caches). The one exception is a **BYOM local `Unregister`** (future) — an explicit, user-initiated removal. @@ -84,9 +84,9 @@ Two behavior changes fall out of this: |---|---|---|---| | D1 | Architecture | **Source/store split.** One owning store; sources are fetch-only. | Preserves `Model*` stability and single ownership; reuses existing index/refresh. | | D2 | Sources return | **`ModelInfo`, not `Model`.** The store owns the `ModelFactory`. | Keeps sources free of `DownloadManager` / `ModelLoadManager` coupling. | -| D3 | Catalog source | **Explicit `CatalogSource` enum field on `ModelInfo`**, not a property-bag entry, and **not** an overload of `model_provider`. | It is correctness-critical (drives dedup/preference), sits on the compare hot path, and is a small closed set. `model_provider` describes *who publishes*; catalog source describes *which catalog served* it. | -| D4 | Duplicate storage | **Shadow variants** inside the alias container; the public API view filters to the preferred copy. | Internal duplicates are harmless (the user controls model selection); the visible list is a filterable `flModelList` view. Keeps each source's full metadata for fallback. | -| D5 | Preference | `local > private > public`, applied by **source-aware variant ordering** so both by-`model_id` lookup and the visible view resolve to the preferred copy. | Small, closed tiebreak; applies only when two sources serve the same `model_id` (the unique model identifier). | +| D3 | Catalog source | **Explicit `CatalogSource` enum field on `ModelInfo`**, not a property-bag entry, and **not** an overload of `model_provider`. | It is correctness-critical (drives preference), sits on the compare hot path, and is a small closed set. `model_provider` describes *who publishes*; catalog source describes *which catalog served* it. | +| D4 | Duplicate storage | **Shadow variants** inside the alias container; public/global enumeration returns every copy, including duplicate `model_id`s, in deterministic catalog sort order. | Keeps each source's full metadata visible and available for fallback without adding a separate deduplication projection. | +| D5 | Preference | `local > private > public`, applied by **source-aware variant ordering** so by-`model_id` lookup resolves to the preferred copy and default selection uses it after the existing cached-first rule. | Small, closed tiebreak; applies only when two sources serve the same `model_id` (the unique model identifier), without hiding either copy from enumeration. | | D6 | Cached-state / non-latest metadata | The local scan attaches `local_path` + cached state to the matching catalog entry. For cached **non-latest** versions absent from the latest cloud fetch, resolve full metadata via the online source's `FetchModelsByIds`. | A downloaded cloud model is not a duplicate — it is the cloud entry with local state attached. Matches current behavior. | | D7 | Private catalog | A **future follow-up**; an additional online catalog source. Its shape, auth, and fetch implementation are **deferred** — the source/store split leaves room for it without a redesign. | Out of initial scope; captured only as a placeholder. | | D8 | Local models | **No separate source now.** The Azure source keeps today's flow — scan the cache (`ScanLocalModels`), fetch live metadata resolving cached ids by-id (`FetchAllModelInfosWithCachedModels`), then `AddLocalModels` attaches `local_path` and synthesizes stubs (`MakeByomModelInfo`) for disk-only models. The only change: **tag those synthesized stubs `kLocal`**. The dedicated **BYOM local catalog** replaces this stub path later. | Least risk; reuses working code. A separate `LocalModelSource` adds a merge/ownership split with no gain while there are no shadows. | @@ -120,8 +120,8 @@ Two behavior changes fall out of this: ┌──────────────────────────────┐ public API ───> │ ModelCatalog : ICatalog │ owns ModelFactory, containers, / C ABI │ - models_ / indices / cache │ indices, caching, refresh; - │ - vector> sources│ to preferred + │ - vector> sources│ all-variant enumeration └──────────────┬───────────────┘ │ composes (fetch-only; return ModelInfo) ┌─────────────────────┼─────────────────────┐ @@ -142,7 +142,8 @@ Two behavior changes fall out of this: `kLocal` on the orphan stubs it synthesizes from the local cache scan. - The store creates a leaf via its owned `ModelFactory` for every `ModelInfo` (including same-`model_id` shadows), groups them by alias into containers, and orders variants source-aware - so the preferred copy wins in `id_index` and the visible view. + so the preferred copy wins in `id_index`, while cached-first default selection uses that order and + enumeration retains all copies. ## Design @@ -174,14 +175,14 @@ Two behavior changes fall out of this: - **`ModelCatalog : ICatalog`** — evolves `BaseModelCatalog`. Holds `std::vector> sources_`, the `ModelFactory`, and the existing store/indices/refresh. Containers may hold same-`model_id` shadow variants; variant ordering is - **source-aware** (preferred first) so `id_index` first-wins and the visible view resolve to the - preferred copy. Adds a local-BYO `Unregister` path that removes a variant (see Removal & fallback). + **source-aware** (preferred first) so `id_index` first-wins, while cached-first default selection + falls back to the preferred copy. Adds a local-BYO `Unregister` path that removes a variant (see Removal & fallback). -### Variant ordering, selection & visibility (shadow variants) +### Variant ordering, selection & enumeration (shadow variants) Shadow variants (same `model_id` from different catalog sources) live inside the alias container's -`variants_` alongside genuine distinct variants. Three mechanisms keep them internally complete while -the public surface stays de-duplicated: +`variants_` alongside genuine distinct variants. Their ordering keeps enumeration deterministic and +preference-based operations unambiguous: - **Ordering — `CompareBestFirst` gains a final tiebreak.** The comparator keeps its existing keys (device priority asc, version desc, created-at desc, `model_id` asc) and appends @@ -190,44 +191,23 @@ the public surface stays de-duplicated: non-duplicates differ earlier and are unaffected. `AddVariant`'s `upper_bound` insert therefore places every shadow in preferred-first order regardless of source insertion order. -- **Internal enumeration — `Variants()` is unchanged and all-inclusive.** It returns every leaf, - including shadows, and remains the accessor used by internal machinery (`RebuildIndex`, - `GetCachedModels` / `GetLoadedModels`, merge/integration). Because `variants_` is preferred-first, - `RebuildIndex`'s first-wins `id_index[model_id]` resolves to the **preferred** leaf automatically. - -- **Public enumeration — new `Model::UniqueVariants()`.** Returns `std::vector` filtered to - one leaf per `model_id`: it walks `variants_` in its existing best-first order (under a single lock) - and keeps the first occurrence of each `model_id` (which, per the tiebreak above, is the - preferred-source copy), skipping later shadows. This is the accessor **both public surfaces** use: - the C API `Model_GetVariantsImpl` (`c_api.cc`) and the REST `GET /v1/models` - `OpenAIListModelsHandler` (`service/models_handlers.cc`) switch from `Variants()` to - `UniqueVariants()`, so neither emits duplicate `model_id`s. The visible `flModelList` / model list is - thus a filtered projection; internal storage keeps the full shadow set for fallback. - - *Why a method, not inline filtering:* it has **two** public callers, so centralizing keeps the - shadow-dedup policy next to the ordering rules rather than duplicated across handlers. It is also - **allocation-neutral** — these sites already call `Variants()` (a heap snapshot vector) and copy - into their output; `UniqueVariants()` returns the same single snapshot under one lock. (A - zero-intermediate callback enumerator was rejected: catalog access is explicitly never - performance-critical.) Internal callers — `RebuildIndex`, `IntegrateVariants`, `GetCachedModels` / - `GetLoadedModels` — keep using the all-inclusive `Variants()`. - -**Default variant selection (`SelectDefaultVariant`) — cached wins.** The existing rule stands: -select the first **cached** variant in best-first order, else `variants_.front()`. Precedence is -therefore *cached-first, then source-preference among equals*: - -- Any **local** model (BYO or a downloaded copy) is locally available, so its leaf is constructed with - its **cached flag set by default**. A local shadow is thus cached and, being highest source - priority, is both first-in-order and cached → selected. -- If a lower-priority source's copy is cached but the preferred-source copy is not (e.g. cached - `public` vs. uncached `private`), the **cached** copy is selected — cached beats source preference, - by design. - -Note a deliberate divergence for shadowed `model_id`s: `id_index[model_id]` (used by -`GetModelVariant`) resolves to the *preferred-source* leaf via first-wins ordering, while the -container's *default selection* may be a *cached* lower-priority leaf. These answer different -questions ("give me the preferred copy of this id" vs. "what does this container act on by default") -and are intentionally allowed to differ. +- **Enumeration — `Variants()` stays all-inclusive on every surface.** It returns every leaf, + including shadows, in the existing best-first catalog order. Internal machinery (`RebuildIndex`, + `GetCachedModels` / `GetLoadedModels`, merge/integration), the C API `Model_GetVariantsImpl` + (`c_api.cc`), and REST `GET /v1/models` `OpenAIListModelsHandler` + (`service/models_handlers.cc`) all continue to use `Variants()`. Consequently, `flModelList` and + `/v1/models` may contain duplicate `model_id`s when multiple sources serve the same model; their + deterministic source-aware order lets consumers distinguish and process every catalog entry. + +- **Direct lookup — preferred source wins for shadows.** Because `variants_` is preferred-first for + otherwise-identical copies, `RebuildIndex`'s first-wins `id_index[model_id]` resolves a shadowed + `model_id` to the highest-priority available source (`local > private > public`). + +- **Default selection — cached still wins.** `SelectDefaultVariant` keeps the existing rule: select + the first cached variant in best-first order, otherwise select `variants_.front()`. Source priority + therefore breaks ties between equally available shadows, but a cached lower-priority source may be + selected over an uncached preferred source. This deliberate distinction preserves direct-ID lookup + preference without changing the container's availability-oriented default behavior. ### Merge algorithm (`ModelCatalog::Populate`) @@ -235,9 +215,9 @@ and are intentionally allowed to differ. alias (`PopulateModels`), and `RebuildIndex`. The **one change**: gather `ModelInfo` from all sources (each stamped by `Source()`) and allow same-`model_id` **shadows** — the dedup key in `IntegrateVariants` becomes `(model_id, catalog_source)` and variant ordering is source-aware -(`local > private > public`) so `id_index` first-wins and the visible view resolve to the preferred -copy. In the initial single-source scope no shadows arise (see *What "local" means*), so this is -dormant foundation. +(`local > private > public`) so `id_index` first-wins, while cached-first default selection uses +that preferred order and enumeration retains all shadows. In the initial single-source scope no shadows arise (see +*What "local" means*), so this is dormant foundation. ### Removal & fallback @@ -274,52 +254,9 @@ carve out this exception. `ModelFactory`, and construct one `ModelCatalog`. `catalog_` stays `std::unique_ptr`. The list is source-agnostic, so future Private / BYOM sources slot in without store changes. -## Delivery phases - -Phase 0–3 are the **initial scope: the Public (Azure) source** (with inline local-cache resolution + -the `kLocal` tag). The **Private catalog** and the dedicated **BYOM local catalog** are **future -follow-ups** — each an independent source that needs no redesign of the store, sources, or public API. - -### Phase 0 — Types & metadata -1. Add `CatalogSource` enum + `CatalogSourcePriority` helper. -2. Add `CatalogSource catalog_source` to `ModelInfo`; round-trip in - `ModelInfoFromJson` / `ModelInfoToJson`. - -### Phase 1 — Source abstraction *(parallel after Phase 0)* -3. New `IModelSource` interface (returns `ModelInfo`). -4. `AzureModelSource` (fetch guts from `AzureModelCatalog`; serves Public and retains the inline - local-cache resolution — `ScanLocalModels` + `GetLiveCatalogOrLocalSnapshot` + `AddLocalModels`, - incl. the snapshot fallback and `CreateCatalogClient` seam). -5. Tag the synthesized orphan stubs `kLocal` in `MakeByomModelInfo` (`azure_model_catalog.cc`) — the - only local-specific change. - -### Phase 2 — Aggregating store *(depends on Phase 1)* -6. `ModelCatalog : ICatalog` — owns `ModelFactory`, `sources_`; reuse group/index/refresh with - source-aware variant ordering, `UniqueVariants()`-backed preferred-only public view, and the - `Unregister` / `RemoveVariant` removal path (foundation; dormant with one source). -7. Implement the merge / leaf-build / shadow-duplicate algorithm above; preserve cache-only mode - and `CatalogCache` save. - -### Phase 3 — Wiring *(depends on Phase 2)* -8. `Manager::Create` builds the `[Public]` source, constructs the factory and `ModelCatalog`. - -### Phase 4 — Tests *(parallel with Phases 2–3)* -9. `FakeModelSource` helper (replaces the `TestCatalog` `FetchModels` override; returns `ModelInfo`). -10. `model_catalog_test.cc`: **local classification** (scanned id matching public → `kPublic` cached - single leaf; orphan id → `kLocal` stub) / preference (`local > private > public`) / shadow-variant - visibility / preferred-only visible view (`UniqueVariants` de-dup) / `SelectDefaultVariant` - precedence (cached-beats-source; local-cached-by-default) / cached-state attachment / - cached-non-latest cloud resolution / fallback-on-unregister via `FakeModelSource` shadows - (`RemoveVariant` compaction + empty-container removal + surviving-shadow re-selection) / - `catalog_source` round-trips through the cache JSON / union-of-aliases across sources. -11. Migrate the Azure catalog tests (`azure_catalog_test.cc` + `azure_model_catalog_test.cc`, incl. the - snapshot-fallback / BYOM-synthesis / dedup cases) → `azure_model_source_test.cc` (fetch guts - unchanged). Keep `catalog_cache_test`, `model_sorting_test`, and `sdk_api/catalog_test` (surface - unchanged). - ## Future follow-ups *(out of initial scope; no redesign required)* -Each is independent and depends only on Phase 3. All slot into the same source/store design. +Each is independent and builds on the initial source/store refactor. All slot into the same design. ### Private catalog - An additional online catalog source (its own `IModelSource`, stamped `kPrivate`), plus any @@ -331,52 +268,10 @@ Each is independent and depends only on Phase 3. All slot into the same source/s `Register` / `Unregister` API. It **replaces** the Azure source's short-term inline local-cache resolution and `kLocal` stubs; `Unregister` wires to the `ModelCatalog` variant-removal path. -### Public visibility of duplicate sources -- Once a real scenario needs it, add a public way for a consumer to enumerate all of a model's - duplicate catalog sources (the shadows the visible view hides). Not built now — the visible view - surfaces only the preferred copy, which is sufficient until a concrete need arises. - -## Affected files - -**New** -- `catalog/model_source.h` (`IModelSource`) -- `catalog/azure_model_source.{h,cc}` -- `catalog/model_catalog.{h,cc}` - -**Modify** -- [src/model_info.h](../src/model_info.h) / [src/model_info.cc](../src/model_info.cc) — - `CatalogSource` enum, `CatalogSourcePriority`, `catalog_source` field + JSON round-trip -- [src/model.h](../src/model.h) / [src/model.cc](../src/model.cc) — source-aware **final tiebreak** - in `CompareBestFirst` (orders genuine duplicates preferred-first, non-duplicates unchanged); new - `UniqueVariants()` preferred-only view for the public list (leaving `Variants()` all-inclusive for - internal use); `RemoveVariant` for local-BYO `Unregister`; local leaves constructed cached-by-default -- [src/catalog/azure_model_catalog.cc](../src/catalog/azure_model_catalog.cc) — `MakeByomModelInfo` - (the disk-only stub synthesizer) stamps `catalog_source = kLocal` on the stubs it - produces (the only local-specific change). `FetchAllModelInfosWithCachedModels` - ([catalog_client.cc](../src/catalog/catalog_client.cc)) — which now only fetches latest + resolves - cached ids by-id — is untouched. -- [src/manager.cc](../src/manager.cc) / [src/manager.h](../src/manager.h) — build the `[Public]` - source (~L325), construct the factory + `ModelCatalog` (factory `CreateModel` at ~L545) -- [src/catalog/catalog_cache.h](../src/catalog/catalog_cache.h) / - [src/catalog/catalog_cache.cc](../src/catalog/catalog_cache.cc) — round-trips `catalog_source` - via `ModelInfo` JSON -- [include/foundry_local/foundry_local_c.h](../include/foundry_local/foundry_local_c.h) / - [src/c_api.cc](../src/c_api.cc) — read-only `catalog_source` (int) on `flModelInfo`; - `Model_GetVariantsImpl` switches from `Variants()` to `UniqueVariants()` so the public list is - de-duplicated. Any Private-catalog C-API surface stays **append-only** and is a future follow-up. -- [src/service/models_handlers.cc](../src/service/models_handlers.cc) — `OpenAIListModelsHandler` - (`GET /v1/models`) switches from `Variants()` to `UniqueVariants()` so it never emits duplicate - `model_id`s across catalog sources -- [CMakeLists.txt](../CMakeLists.txt) — add/remove sources - -**Remove / absorb** -- `catalog/base_model_catalog.{h,cc}` → `catalog/model_catalog.{h,cc}` -- `catalog/azure_model_catalog.{h,cc}` → `catalog/azure_model_source.{h,cc}` (keeps the inline - local-cache resolution: `ScanLocalModels` + `GetLiveCatalogOrLocalSnapshot` + `AddLocalModels` / - `MakeByomModelInfo`, incl. the snapshot fallback and `CreateCatalogClient` seam) -- `FetchAllModelInfosWithCachedModels` ([catalog/catalog_client.cc](../src/catalog/catalog_client.cc)) - **stays** — reused by `AzureModelSource`; unchanged (stub synthesis lives in `MakeByomModelInfo`). - `ICatalogClient` in `catalog_client.h` stays (used by `AzureModelSource`). +### Public identification of duplicate sources +- Enumeration already exposes all duplicate catalog entries. Once a real scenario needs it, add a + richer public source identifier beyond the read-only `catalog_source` value so consumers can + present or select duplicate sources more explicitly. ## Verification @@ -390,8 +285,8 @@ Each is independent and depends only on Phase 3. All slot into the same source/s - **Included (initial scope)**: the Public (Azure) source with inline local-cache resolution (cached `kPublic` entries + short-term `kLocal` orphan stubs, per *What "local" means*); the - shadow / preference / `UniqueVariants` machinery and the `Unregister` / `RemoveVariant` path as - dormant foundation. + shadow / preference / duplicate-preserving enumeration machinery and the `Unregister` / + `RemoveVariant` path as dormant foundation. - **Excluded (future follow-ups)**: the **Private** catalog and the dedicated **BYOM local** catalog (which replaces the interim `kLocal` stubs and adds `Register` / `Unregister`). Both are new sources, not redesigns; details TBD. diff --git a/sdk_v2/cpp/src/c_api.cc b/sdk_v2/cpp/src/c_api.cc index c87bce99a..2479a0d29 100644 --- a/sdk_v2/cpp/src/c_api.cc +++ b/sdk_v2/cpp/src/c_api.cc @@ -857,9 +857,7 @@ FL_API_STATUS_IMPL(Model_GetVariantsImpl, const flModel* model, flModelList** ou return MakeStatus(FOUNDRY_LOCAL_ERROR_INVALID_ARGUMENT, "null argument"); } - // Public list is de-duplicated to one leaf per model_id (preferred-source copy); internal - // storage keeps all shadow variants. See MultiCatalogSupportPlan.md. - auto variants = AsImpl(model)->UniqueVariants(); + auto variants = AsImpl(model)->Variants(); auto list = std::make_unique(); list->items.reserve(variants.size()); diff --git a/sdk_v2/cpp/src/catalog/model_catalog.h b/sdk_v2/cpp/src/catalog/model_catalog.h index bdd32dd0b..948cbcfd9 100644 --- a/sdk_v2/cpp/src/catalog/model_catalog.h +++ b/sdk_v2/cpp/src/catalog/model_catalog.h @@ -21,7 +21,7 @@ namespace fl { /// Owns the ModelFactory and every Model instance, plus a list of fetch-only IModelSources /// (one per catalog type). It merges ModelInfo across sources into alias containers, keeps /// same-model_id copies from different sources as shadow variants (ordered preferred-source -/// first), and serves a filtered preferred-only public API view. Fetch lives in the sources; +/// first), and serves every stored variant in deterministic catalog order. Fetch lives in the sources; /// store / query / index / create / cache / refresh live here. /// /// Model ownership: the catalog owns all Model instances via unique_ptr in models_. These diff --git a/sdk_v2/cpp/src/model.cc b/sdk_v2/cpp/src/model.cc index e59bad18f..c9cd8aca3 100644 --- a/sdk_v2/cpp/src/model.cc +++ b/sdk_v2/cpp/src/model.cc @@ -15,7 +15,6 @@ #include #include -#include namespace fl { @@ -257,28 +256,6 @@ std::vector Model::Variants() const { return result; } -std::vector Model::UniqueVariants() const { - std::lock_guard lock(state_mutex_); - - if (!IsContainer()) { - return {const_cast(this)}; - } - - // variants_ is kept best-first (AddVariant's ordered insert), and same-model_id shadow - // variants are ordered preferred-source first by CompareBestFirst's final tiebreak. Keep - // the first occurrence of each model_id so the visible list is the preferred-source copy. - std::vector result; - result.reserve(variants_.size()); - std::unordered_set seen_ids; - for (auto& v : variants_) { - if (seen_ids.insert(v->Info().model_id).second) { - result.push_back(const_cast(v.get())); - } - } - - return result; -} - size_t Model::VariantCount() const { std::lock_guard lock(state_mutex_); return variants_.size(); diff --git a/sdk_v2/cpp/src/model.h b/sdk_v2/cpp/src/model.h index cda129b54..429316bce 100644 --- a/sdk_v2/cpp/src/model.h +++ b/sdk_v2/cpp/src/model.h @@ -97,13 +97,6 @@ class Model { /// `std::unique_ptr::get() const → T*` idiom. std::vector Variants() const; - /// Like Variants() but filtered to one leaf per model_id — the visible, de-duplicated view - /// used by the public surfaces (C API GetVariants, REST GET /v1/models). Walks the container's - /// best-first variant order under a single lock and keeps the first occurrence of each model_id, - /// which (per CompareBestFirst's source-priority tiebreak) is the preferred-source copy; later - /// same-model_id shadows are skipped. For a leaf, returns {this}. - std::vector UniqueVariants() const; - // --- Query methods --- bool IsCached() const; diff --git a/sdk_v2/cpp/src/service/models_handlers.cc b/sdk_v2/cpp/src/service/models_handlers.cc index a464dccf5..30270686e 100644 --- a/sdk_v2/cpp/src/service/models_handlers.cc +++ b/sdk_v2/cpp/src/service/models_handlers.cc @@ -160,10 +160,9 @@ class OpenAIListModelsHandler : public HttpRequestHandler { auto models = ctx_.catalog.ListModels(); nlohmann::json data = nlohmann::json::array(); - // List individual variants so the client knows exactly which model_id to use. - // UniqueVariants() de-duplicates shadow model_ids to the preferred-source copy. + // List every stored variant in catalog order, including duplicate model_ids from different sources. for (const auto* model : models) { - for (const auto* variant : model->UniqueVariants()) { + for (const auto* variant : model->Variants()) { const auto& info = variant->Info(); int64_t created = 0; auto it = info.int_properties.find(FOUNDRY_LOCAL_MODEL_PROP_CREATED_AT_UNIX_INT); diff --git a/sdk_v2/cpp/test/internal_api/c_api_test.cc b/sdk_v2/cpp/test/internal_api/c_api_test.cc index 5fc46d302..3db7bbdbe 100644 --- a/sdk_v2/cpp/test/internal_api/c_api_test.cc +++ b/sdk_v2/cpp/test/internal_api/c_api_test.cc @@ -2,13 +2,20 @@ // Licensed under the MIT License. #include "internal_api/c_api_test_helpers.h" +#include "c_api_types.h" +#include "catalog/model_catalog.h" +#include "catalog/model_source.h" +#include "internal_api/test_helpers.h" + #include #include #include #include #include +#include #include #include +#include #include // All tests go through the vtable obtained from FoundryLocalGetApi(). @@ -19,6 +26,25 @@ using fl::test::GetApi; using fl::test::IsOk; using fl::test::StatusGuard; +namespace { + +class FixedModelSource final : public fl::IModelSource { + public: + FixedModelSource(fl::CatalogSource source, fl::ModelInfo model) : source_(source), model_(std::move(model)) { + model_.catalog_source = source; + } + + fl::CatalogSource Source() const override { return source_; } + std::string Name() const override { return "fixed-source"; } + std::vector FetchModels() const override { return {model_}; } + + private: + fl::CatalogSource source_; + fl::ModelInfo model_; +}; + +} // namespace + // ======================================================================== // Exports & Version // ======================================================================== @@ -320,6 +346,56 @@ TEST(CApiTest, GetModelsFromCatalog) { api->Manager_Release(mgr); } +TEST(CApiTest, ModelGetVariantsReturnsDuplicateIdsInSourceOrder) { + const flApi* api = GetApi(); + ASSERT_NE(api, nullptr); + const flModelApi* model_api = api->GetModelApi(); + + fl::ModelInfo duplicate; + duplicate.model_id = "duplicate-model:1"; + duplicate.name = "duplicate-model"; + duplicate.version = 1; + duplicate.alias = "duplicate"; + + std::vector> sources; + sources.push_back(std::make_unique(fl::CatalogSource::kPublic, duplicate)); + sources.push_back(std::make_unique(fl::CatalogSource::kPrivate, duplicate)); + sources.push_back(std::make_unique(fl::CatalogSource::kLocal, duplicate)); + + fl::test::FakeServiceBindings services; + fl::ModelCatalog catalog( + "duplicate-source-catalog", std::move(sources), + [&services](fl::ModelInfo info) { + return fl::Model::FromModelInfo(std::move(info), services.download_manager, services.model_load_manager); + }, + services.logger); + + fl::Model* grouped_model = catalog.GetModel("duplicate"); + ASSERT_NE(grouped_model, nullptr); + + flModelList* variants = nullptr; + ASSERT_FL_OK(api, model_api->GetVariants(AsHandle(grouped_model), &variants)); + std::unique_ptrModelList_Release)> variants_guard(variants, api->ModelList_Release); + ASSERT_NE(variants, nullptr); + ASSERT_EQ(api->ModelList_Size(variants), 3u); + + const std::vector expected_sources = { + fl::CatalogSource::kLocal, + fl::CatalogSource::kPrivate, + fl::CatalogSource::kPublic, + }; + for (size_t i = 0; i < expected_sources.size(); ++i) { + flModel* variant = api->ModelList_GetAt(variants, i); + ASSERT_NE(variant, nullptr); + + const flModelInfo* info = nullptr; + ASSERT_FL_OK(api, model_api->GetInfo(variant, &info)); + ASSERT_NE(info, nullptr); + EXPECT_STREQ(model_api->Info_GetId(info), "duplicate-model:1"); + EXPECT_EQ(model_api->Info_GetCatalogSource(info), static_cast(expected_sources[i])); + } +} + TEST(CApiTest, GetModelVersionsNullCatalogFails) { const flApi* api = GetApi(); ASSERT_NE(api, nullptr); diff --git a/sdk_v2/cpp/test/internal_api/model_catalog_test.cc b/sdk_v2/cpp/test/internal_api/model_catalog_test.cc index f79f087ee..33566c432 100644 --- a/sdk_v2/cpp/test/internal_api/model_catalog_test.cc +++ b/sdk_v2/cpp/test/internal_api/model_catalog_test.cc @@ -3,8 +3,8 @@ // // Tests for ModelCatalog — the aggregating store behind ICatalog. Fetch lives in // IModelSources (here a FakeModelSource); the store owns the ModelFactory, groups variants -// by alias, keeps same-model_id shadows from different sources, and serves a preferred-only -// public view (UniqueVariants). Also covers source preference, SelectDefaultVariant +// by alias, keeps same-model_id variants from different sources visible in deterministic order, +// and resolves by-ID lookups to the preferred source. Also covers SelectDefaultVariant // precedence, the Unregister / RemoveVariant fallback path, and catalog_source JSON round-trip. // #include "catalog/model_catalog.h" @@ -369,7 +369,7 @@ TEST_F(ModelCatalogTest, GetModelVariantIdIntegrationPreservesPriorityOrdering) } // ======================================================================== -// Multi-source: union of aliases, shadow variants, preference, visibility +// Multi-source: union of aliases, duplicate variants, preference, visibility // ======================================================================== TEST_F(ModelCatalogTest, MultipleSourcesUnionAliases) { @@ -381,22 +381,46 @@ TEST_F(ModelCatalogTest, MultipleSourcesUnionAliases) { EXPECT_EQ(list.size(), 2u); } -TEST_F(ModelCatalogTest, ShadowVariantsKeptInternallyButHiddenFromUniqueView) { - // Same model_id served by public and local sources → shadow variants. - default_source_->AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3")); +TEST_F(ModelCatalogTest, DuplicateModelIdsRemainVisibleInSortedSourceOrderAndByIdPrefersSource) { + default_source_->AddModel( + MakeInfo("phi-3-mini-gpu:2", "phi-3-mini", 2, "phi-3", "", CatalogSource::kPublic)); + default_source_->AddModel( + MakeInfo("phi-3-mini-cpu:1", "phi-3-mini", 1, "phi-3", "", CatalogSource::kPublic)); + default_source_->AddModel( + MakeInfo("phi-3-mini-gpu:1", "phi-3-mini", 1, "phi-3", "", CatalogSource::kPublic)); + auto* local = AddSource(CatalogSource::kLocal, "local-source"); - local->AddModel(MakeInfo("phi-3-mini:1", "phi-3-mini", 1, "phi-3", "/cache/phi", CatalogSource::kLocal)); + local->AddModel( + MakeInfo("phi-3-mini-gpu:1", "phi-3-mini", 1, "phi-3", "", CatalogSource::kLocal)); - auto* container = Catalog().GetModel("phi-3"); + auto* priv = AddSource(CatalogSource::kPrivate, "private-source"); + priv->AddModel( + MakeInfo("phi-3-mini-gpu:1", "phi-3-mini", 1, "phi-3", "", CatalogSource::kPrivate)); + + auto& catalog = Catalog(); + auto* container = catalog.GetModel("phi-3"); ASSERT_NE(container, nullptr); - // Internal enumeration keeps both shadows. - EXPECT_EQ(container->Variants().size(), 2u); - // Public view collapses to one leaf per model_id. - auto unique = container->UniqueVariants(); - ASSERT_EQ(unique.size(), 1u); - EXPECT_EQ(unique.front()->Info().catalog_source, CatalogSource::kLocal) - << "The preferred (local) shadow should be the visible copy."; + const std::vector> expected = { + {"phi-3-mini-gpu:2", CatalogSource::kPublic}, + {"phi-3-mini-gpu:1", CatalogSource::kLocal}, + {"phi-3-mini-gpu:1", CatalogSource::kPrivate}, + {"phi-3-mini-gpu:1", CatalogSource::kPublic}, + {"phi-3-mini-cpu:1", CatalogSource::kPublic}, + }; + + const auto variants = container->Variants(); + ASSERT_EQ(variants.size(), expected.size()); + + for (size_t i = 0; i < expected.size(); ++i) { + EXPECT_EQ(variants[i]->Info().model_id, expected[i].first); + EXPECT_EQ(variants[i]->Info().catalog_source, expected[i].second); + } + + auto* by_id = catalog.GetModelVariant("phi-3-mini-gpu:1"); + ASSERT_NE(by_id, nullptr); + EXPECT_EQ(by_id->Info().catalog_source, CatalogSource::kLocal) + << "By-ID lookup should still resolve duplicate model IDs to the preferred source."; } TEST_F(ModelCatalogTest, IdIndexResolvesToPreferredSource) { diff --git a/sdk_v2/cpp/test/internal_api/web_service_test.cc b/sdk_v2/cpp/test/internal_api/web_service_test.cc index b43f2660d..6715be5b5 100644 --- a/sdk_v2/cpp/test/internal_api/web_service_test.cc +++ b/sdk_v2/cpp/test/internal_api/web_service_test.cc @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -288,6 +289,38 @@ TEST_F(WebServiceTest, OpenAIListModelsPopulatesPublisher) { EXPECT_TRUE(found) << "alpha-model:1 not found in response: " << j.dump(2); } +TEST_F(WebServiceTest, OpenAIListModelsPreservesDuplicateIdsInCatalogSourceOrder) { + test::MockCatalog catalog; + + for (const auto& [source, publisher] : + std::vector>{ + {CatalogSource::kPublic, "public-owner"}, + {CatalogSource::kLocal, "local-owner"}, + {CatalogSource::kPrivate, "private-owner"}, + }) { + auto info = test::MakeTestModelInfo("duplicate-model", publisher); + info.catalog_source = source; + catalog.AddModel(Model::FromModelInfo(std::move(info), svc_.download_manager, svc_.model_load_manager)); + } + + WebService service(catalog, *logger_, "/tmp/test-cache", *model_load_manager_, *session_manager_, *null_telemetry_, + []() {}); + const auto urls = service.Start({"http://127.0.0.1:0"}); + ASSERT_EQ(urls.size(), 1u); + + const auto response = json::parse(TestHttpGet(urls[0] + "/v1/models")); + const auto& models = response["data"]; + ASSERT_EQ(models.size(), 3u) << "Response: " << response.dump(2); + + const std::vector expected_owners = {"local-owner", "private-owner", "public-owner"}; + for (size_t i = 0; i < expected_owners.size(); ++i) { + EXPECT_EQ(models[i]["id"], "duplicate-model:1") << "Response: " << response.dump(2); + EXPECT_EQ(models[i]["owned_by"], expected_owners[i]) << "Response: " << response.dump(2); + } + + service.Stop(); +} + // ======================================================================== // GET /v1/models/{name} — OpenAI-compatible retrieve // ========================================================================