The Implementation for Bring Your Own Local Model (BYOM) API - #928
The Implementation for Bring Your Own Local Model (BYOM) API#928Selena Yang (selenayang888) wants to merge 24 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Introduce ABI v3 with Info_Clone while preserving v1/v2 tables, restore independent C++ ModelInfo copy construction and assignment, and keep explicit CPU model loading on OGA's default provider. Add C ABI and C++ copy-semantics tests.
There was a problem hiding this comment.
Pull request overview
Adds persistent registration and inference support for local ONNX models through new local catalog and C/C++ APIs.
Changes:
- Adds local model registration, persistence, metadata, and lifecycle handling.
- Extends versioned C/C++ APIs with catalog selection and mutable
ModelInfo. - Adds unit tests and separates legacy cached models from explicitly registered BYOM models.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
sdk_v2/cpp/test/internal_api/model_info_test.cc |
Tests property-bag persistence. |
sdk_v2/cpp/test/internal_api/model_info_accessors_test.cc |
Tests ModelInfo copy semantics. |
sdk_v2/cpp/test/internal_api/local_model_catalog_test.cc |
Tests local catalog behavior. |
sdk_v2/cpp/test/internal_api/c_api_test.cc |
Tests versioned cloning API. |
sdk_v2/cpp/test/internal_api/azure_catalog_test.cc |
Updates unresolved-cache expectations. |
sdk_v2/cpp/test/CMakeLists.txt |
Adds local catalog tests. |
sdk_v2/cpp/src/model.h |
Adds local-model lifecycle state. |
sdk_v2/cpp/src/model.cc |
Implements local loading and unregistering. |
sdk_v2/cpp/src/model_info.h |
Declares property-bag utilities. |
sdk_v2/cpp/src/model_info.cc |
Implements property persistence. |
sdk_v2/cpp/src/manager.h |
Adds multiple catalog support. |
sdk_v2/cpp/src/manager.cc |
Creates public and local catalogs. |
sdk_v2/cpp/src/inferencing/session/session.cc |
Uses collision-safe runtime IDs. |
sdk_v2/cpp/src/inferencing/generative/genai_model_instance.cc |
Revises provider overrides. |
sdk_v2/cpp/src/catalog/local_model_catalog.h |
Defines the local catalog. |
sdk_v2/cpp/src/catalog/local_model_catalog.cc |
Implements registration persistence. |
sdk_v2/cpp/src/catalog/catalog_client.h |
Documents explicit BYOM registration. |
sdk_v2/cpp/src/catalog/catalog_client.cc |
Removes synthesized BYOM entries. |
sdk_v2/cpp/src/catalog/base_model_catalog.h |
Adds catalog types and tombstones. |
sdk_v2/cpp/src/catalog/base_model_catalog.cc |
Implements model deactivation. |
sdk_v2/cpp/src/catalog/azure_model_catalog.cc |
Filters legacy local entries. |
sdk_v2/cpp/src/catalog.h |
Extends the catalog interface. |
sdk_v2/cpp/src/c_api.cc |
Implements versioned BYOM C APIs. |
sdk_v2/cpp/include/foundry_local/foundry_local_cpp.inline.h |
Implements C++ wrappers. |
sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h |
Exposes new C++ APIs. |
sdk_v2/cpp/include/foundry_local/foundry_local_c.h |
Defines C API v3. |
sdk_v2/cpp/CMakeLists.txt |
Builds the local catalog source. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
sdk_v2/cpp/src/model.cc:351
- For a deferred registration whose directory/config is still missing, this reports 100% and succeeds even though
IsCached()remains false. That breaks the usualDownload()contract and lets callers proceed as if the model were available. Check availability and return an error until the externally managed assets exist.
if (external_registration_) {
if (progress_cb) {
progress_cb(100.0f);
}
return;
sdk_v2/cpp/src/catalog/base_model_catalog.cc:66
- On refresh this only appends aliases absent from the current active set; it never deactivates entries removed from the fetched source. Because the local registration index is cross-process and file-locked, an unregister/re-register in another process remains stale here forever (even after the four-hour refresh). Reconcile local entries by stable registration ID, tombstoning missing/replaced registrations while preserving pointer safety.
models_.push_back({std::make_unique<Model>(std::move(model)), true});
sdk_v2/cpp/src/catalog/local_model_catalog.cc:476
- The registration index is documented as authoritative, yet failure to create this optional sidecar aborts registration. Consequently, a valid model on a read-only/shared filesystem cannot be registered from its arbitrary path. Make sidecar creation best-effort (with a warning) or store generated metadata under app data instead.
std::ofstream stream(temp_path, std::ios::binary | std::ios::trunc);
if (!stream) {
FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL,
"failed to write model_metadata.yml beside BYOM assets: " + registration.model_path);
sdk_v2/cpp/src/c_api.cc:763
- The new public C entry points are not exercised by
c_api_test.cc; the added catalog tests callLocalModelCatalogdirectly. Add an end-to-end C API test that obtains the local catalog through the versioned manager table, creates metadata, registers/lists/unregisters a model, and verifies handle lifetime and asset preservation.
*out_model = AsHandle<flModel>(catalog->impl.RegisterModel(*AsImpl(model_info)));
Baiju Meswani (baijumeswani)
left a comment
There was a problem hiding this comment.
Will add more comments soon.
| std::function<std::optional<ModelInfo>()> prepare_callback) { | ||
| auto model = FromModelInfo(std::move(info), std::move(local_path), download_manager, model_load_manager); | ||
| model.external_registration_ = true; | ||
| model.runtime_model_id_ = "local/" + model.Info().model_id; |
There was a problem hiding this comment.
do we need to treat this differently to any other model id? the more differences we have between how models from public/private/local are treated the more complicated the system is.
There was a problem hiding this comment.
I standardized the public local-model ID to <alias>:<version>. A private local/<registration_id> load-manager key remains to distinguish re-registration of the same ID while preserving outstanding handles. Removing it would require redesigning the model lifecycle. Would keeping identical public ID semantics with a private generation key address your concern?
There was a problem hiding this comment.
nit: the id is typically <name>:<version> as the name has EP/device details like 'generic-gpu' in it but alias does not, and when picking the latest version we need to do that on the per-name basis not per-alias.
the id should always be unique (at least within a catalog). can we use that instead of a separate registration_id?
not sure what you mean by "identical public ID semantics with a private generation key". Can you elaborate?
…_mb, not a bytes property
Move UTC timestamp formatting into a shared utility, reuse it for local model registration and cross-process lock metadata, and add focused unit tests.
…ve registrations of local models can persist across application restarts/runs
The implementation to register a local ONNX model from an arbitrary filesystem path and make it available through the standard Catalog/Model/Inference api.