Skip to content

The Implementation for Bring Your Own Local Model (BYOM) API - #928

Open
Selena Yang (selenayang888) wants to merge 22 commits into
mainfrom
syang/bring-your-own-model
Open

The Implementation for Bring Your Own Local Model (BYOM) API#928
Selena Yang (selenayang888) wants to merge 22 commits into
mainfrom
syang/bring-your-own-model

Conversation

@selenayang888

Copy link
Copy Markdown
Contributor

The implementation to register a local ONNX model from an arbitrary filesystem path and make it available through the standard Catalog/Model/Inference api.

@vercel

vercel Bot commented Jul 30, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
foundry-local Ready Ready Preview Aug 20, 2026 11:02pm

Request Review

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.
@selenayang888
Selena Yang (selenayang888) marked this pull request as ready for review August 6, 2026 01:41
Copilot AI balanced review requested due to automatic review settings August 6, 2026 01:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread sdk_v2/cpp/src/catalog/local_model_catalog.cc Outdated
Comment thread sdk_v2/cpp/src/inferencing/generative/genai_model_instance.cc Outdated
Comment thread sdk_v2/cpp/include/foundry_local/foundry_local_c.h Outdated
Comment thread sdk_v2/cpp/include/foundry_local/foundry_local_c.h Outdated
Comment thread sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h Outdated
Comment thread sdk_v2/cpp/src/catalog/base_model_catalog.cc Outdated
Comment thread sdk_v2/cpp/test/internal_api/azure_catalog_test.cc
Comment thread sdk_v2/cpp/test/internal_api/local_model_catalog_test.cc

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 usual Download() 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 call LocalModelCatalog directly. 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)));

Comment thread sdk_v2/cpp/src/model.cc

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming that the SDKs also need adding to this PR

virtual std::unique_ptr<IModel> RegisterModel(const ModelInfo&) {
throw Error("models can only be registered in a local catalog", FOUNDRY_LOCAL_ERROR_INVALID_ARGUMENT);
}
virtual void UnregisterModel(const std::string&) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest "Deregister"

@selenayang888 Selena Yang (selenayang888) Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We retained UnregisterModel/UnregisterModel pair and precisely describes removing the catalog registration without deleting the model assets. If there is a project-wide naming convention requiring DeregisterModel, I can apply it consistently across the C and C++ APIs before merge.

Comment thread sdk_v2/cpp/src/catalog/local_model_catalog.cc Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add more comments soon.

Comment thread sdk_v2/cpp/src/c_api.cc Outdated
Comment thread sdk_v2/cpp/src/c_api.cc Outdated
Comment thread sdk_v2/cpp/src/c_api.cc Outdated
Comment thread sdk_v2/cpp/include/foundry_local/foundry_local_c.h Outdated
Comment thread sdk_v2/cpp/include/foundry_local/foundry_local_c.h Outdated
Comment on lines +728 to +731
FL_API_STATUS(Manager_GetCatalogByType, _In_ const flManager* manager, flCatalogType catalog_type,
_Outptr_ flCatalog** out_catalog);
FL_API_STATUS(Manager_GetCatalogByName, _In_ const flManager* manager, _In_ const char* catalog_name,
_Outptr_ flCatalog** out_catalog);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something the user needs to know about?

Internally we could have a local catalog that we automatically use for RegisterModel, but that seems like it could stay an implementation detail and there's just a single user facing flCatalog.

Ideally the internals of FL aggregate across the various catalogs available (public/private/local) and the user doesn't need to care about that detail. V1 had a catalog aggregator class that I didn't port across as it hadn't been used yet. We could potentially add that concept back in.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I slightly prefer having separate catalogs per type (public, local, private - if that materializes). Because aggregate views can cause id/name conflicts and it becomes harder for consumers to tell which catalog a particular model came from. We then would need to expose some other model attributes or query-able catalog arguments.

@skottmckay Scott McKay (skottmckay) Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate catalogs at the public API level adds overhead to the SDK user as they can't simply deal with a unified meta-catalog. Do we think that cost is outweighed by the benefit of manually being able to resolve a clash with the model id if it occurred and a default priority of local > private > cloud was used?

I could understand adding a metadata entry to the model for the catalog type. Low cost, ignored by vast majority of SDK users, available if needed using existing API.

Thinking through potential clashes:

  • local with cloud
    • local is higher priority as manually added
    • if you later added that to a cloud repo and want to prefer the cloud version you can unregister the local model
      • local model instance will be the one you're getting by default, you can check the metadata for catalog type if needed (although model location is likely just as good a signal as we're not putting those in our cloud model download cache currently) to know it's a local model that can be unregistered
        • but equally the unregister can return an error saying 'not a local model' as internally we'd still have separate catalogs
  • private with public
    • private is higher priority
      • if you put something in your private catalog assumably it should be preferred over public
        • remove from private catalog if that's not the case

And I would expect an id clash to be an edge case so we should try not to complicate the API for that if we have other ways of handling it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be scenarios where the application wants all three versions of the same model from {public, private, local} and present to their users 1 or all 3 for different scenarios.
There may also be a scenario where the application has a model in the private model catalog before we make it available in the public catalog. Which could then force application to change the name (if they wanted to use both).

Separate catalogs at the public API level adds overhead to the SDK user as they can't simply deal with a unified meta-catalog.

I like the idea of an aggregate catalog, but only as a view of the 3 distinct catalogs. Not as a catalog itself. For example, we could create a helper/function called manager.GetAggregateCatalogView() which would then aggregate the 3 catalogs and present them based on some priority strategy.

This was one of the problems I thought the aggregate catalog had before in the v1 core which got surfaced when we did the private model catalog development.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also the scenario where one could have multiple private catalogs. Then the aggregation would become more involved and conflict resolution may require configurable strategy.

@selenayang888 Selena Yang (selenayang888) Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Scott McKay (@skottmckay) and Baiju Meswani (@baijumeswani). My understanding is that we agree the underlying public, local, and future private catalogs should remain distinct, but we have not agreed on the user-facing view or collision behavior.

For this PR, I propose:

  • Keep GetCatalog() returning the existing public catalog to preserve current behavior.
  • Expose the local registration catalog separately.
  • Keep RegisterModel() and UnregisterModel() scoped to the local catalog.
  • Do not add aggregation in this PR.
  • Consider a read-only GetAggregateCatalogView() in a follow-up after defining provenance and collision semantics.

Could you confirm agreement on these two decisions?

  1. Can this PR ship with separate public and local catalogs, with aggregation deferred to a follow-up?
  2. For accessing the local catalog, should we keep GetCatalog(CatalogType::Local), or use a narrower GetLocalCatalog() API? The explicit accessor avoids treating catalog kinds as a closed enum, which may not scale if multiple private catalogs are supported.

Comment thread sdk_v2/cpp/include/foundry_local/foundry_local_c.h Outdated
Comment thread sdk_v2/cpp/include/foundry_local/foundry_local_c.h Outdated
Comment thread sdk_v2/cpp/src/catalog/local_model_catalog.cc Outdated
Comment thread sdk_v2/cpp/src/catalog/local_model_catalog.cc Outdated
Comment thread sdk_v2/cpp/src/catalog/local_model_catalog.cc Outdated
Comment thread sdk_v2/cpp/src/catalog/local_model_catalog.cc Outdated
Comment thread sdk_v2/cpp/src/catalog/local_model_catalog.h Outdated
Comment thread sdk_v2/cpp/src/manager.cc Outdated
Comment thread sdk_v2/cpp/src/catalog/local_model_catalog.cc Outdated
Comment thread sdk_v2/cpp/src/model.h Outdated
Comment thread sdk_v2/cpp/src/model.h Outdated
Comment thread sdk_v2/cpp/src/model.cc Outdated
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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread sdk_v2/cpp/src/model.h Outdated
Comment thread sdk_v2/cpp/src/model_info.h Outdated
Comment thread sdk_v2/cpp/src/catalog/local_model_catalog.cc Outdated
Comment thread sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h Outdated
Comment thread sdk_v2/cpp/src/inferencing/model_load_manager.cc
Comment on lines +728 to +731
FL_API_STATUS(Manager_GetCatalogByType, _In_ const flManager* manager, flCatalogType catalog_type,
_Outptr_ flCatalog** out_catalog);
FL_API_STATUS(Manager_GetCatalogByName, _In_ const flManager* manager, _In_ const char* catalog_name,
_Outptr_ flCatalog** out_catalog);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I slightly prefer having separate catalogs per type (public, local, private - if that materializes). Because aggregate views can cause id/name conflicts and it becomes harder for consumers to tell which catalog a particular model came from. We then would need to expose some other model attributes or query-able catalog arguments.

Comment thread sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h Outdated
Comment thread sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h Outdated
Comment thread sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h Outdated
Comment thread sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h Outdated
Move UTC timestamp formatting into a shared utility, reuse it for local model registration and cross-process lock metadata, and add focused unit tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants