fix(customizer): namespace backend schemas to fix OpenAPI name collision - #737
Merged
albcui merged 2 commits intoJul 17, 2026
Merged
Conversation
albcui
marked this pull request as ready for review
July 16, 2026 20:11
albcui
force-pushed
the
albcui/aalgo-351-customizer-openapi-schema-name-collision
branch
from
July 16, 2026 20:11
9bb1a9d to
c17ac4c
Compare
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (14)
📝 WalkthroughWalkthroughOpenAPI normalization now supports strict or warning-based schema collision handling. Shared namespaced Pydantic bases are adopted across Automodel, RL, and Unsloth, generated contracts use backend-specific schemas, and ChangesSchema normalization and namespacing
Canonical backend format export
Sequence Diagram(s)sequenceDiagram
participant PluginConfig
participant process_plugin_specs
participant apply_schema_fixes
participant tweak_spec
participant OpenAPISpec
PluginConfig->>process_plugin_specs: provide strict_schema_collisions
process_plugin_specs->>apply_schema_fixes: split strict and lenient specifications
apply_schema_fixes->>tweak_spec: pass strict_collisions
tweak_spec->>OpenAPISpec: normalize schemas and rewrite references
OpenAPISpec-->>tweak_spec: return normalized specification or raise ValueError
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/nmp_customization_common/src/nmp/customization_common/schema.py (1)
26-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop
from __future__ import annotationshere. There are no forward refs in this module, and keepingClassVar[str | None]concrete avoids stringized runtime annotations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nmp_customization_common/src/nmp/customization_common/schema.py` around lines 26 - 28, Remove the `from __future__ import annotations` import from this module, leaving the existing `ClassVar` import and concrete `ClassVar[str | None]` annotations unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nmp_common/src/nmp/common/api/utils.py`:
- Around line 179-202: Update the schema-collision handling around the
`by_target` loop and `rename_map` rebuild so each normalized name explicitly
selects and preserves the first-seen key from `old_keys`, including when that
key differs from the normalized name. Rebuild the normalized schema entry from
that representative rather than allowing an existing `Foo` key to override an
earlier `module__Foo`; retain identical-content deduplication and collision
reporting behavior.
In `@packages/nmp_customization_common/src/nmp/customization_common/schema.py`:
- Around line 42-45: The dynamic class creation in the metaclass `__new__`
currently changes `__qualname__` without ensuring the generated name is
importable. Preserve the original qualname or bind the generated alias in the
defining module, and add a pickle round-trip test covering the dynamically
prefixed class.
---
Nitpick comments:
In `@packages/nmp_customization_common/src/nmp/customization_common/schema.py`:
- Around line 26-28: Remove the `from __future__ import annotations` import from
this module, leaving the existing `ClassVar` import and concrete `ClassVar[str |
None]` annotations unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a71b0a6c-8c82-4a39-b152-112779d64ecf
📒 Files selected for processing (10)
packages/nmp_common/src/nmp/common/api/utils.pypackages/nmp_common/tests/api/test_utils_openapi_spec.pypackages/nmp_customization_common/src/nmp/customization_common/schema.pypackages/nmp_customization_common/tests/test_schema.pyplugins/nemo-automodel/src/nemo_automodel_plugin/schema.pyplugins/nemo-customizer/openapi/openapi.yamlplugins/nemo-unsloth/src/nemo_unsloth_plugin/schema.pyscript/generate_openapi_spec.pyservices/core/models/src/nmp/core/models/schemas.pyservices/unsloth/src/nmp/unsloth/schemas.py
Contributor
|
Should we update the RL parameters so that they are also namespaced for consistency even though there is no failing collisions there? |
Contributor
|
albcui
force-pushed
the
albcui/aalgo-351-customizer-openapi-schema-name-collision
branch
from
July 16, 2026 20:28
c17ac4c to
393f443
Compare
Contributor
Author
Yea good point, RL should get the same treatment. |
albcui
force-pushed
the
albcui/aalgo-351-customizer-openapi-schema-name-collision
branch
from
July 16, 2026 21:02
393f443 to
9b7ea2c
Compare
mckornfield
approved these changes
Jul 17, 2026
mckornfield
left a comment
mckornfield
left a comment
Contributor
There was a problem hiding this comment.
personally I'd be for simplifying and just making exploding the default, but maybe that's a follow up?
soluwalana
approved these changes
Jul 17, 2026
Automodel and Unsloth each define models named TrainingSpec (plus LoRAParams, DatasetSpec, ScheduleSpec, BatchSpec, OptimizerSpec, OutputRequest, OutputResponse); the RL backend likewise defines OutputRequest / OutputResponse. Because all backends mount into one /apis/customization FastAPI app, Pydantic's module-qualified $defs keys were collapsed by the platform's schema-name normalizer, so e.g. both *JobInput.training ended up $ref-ing a single TrainingSpec — Unsloth's request contract silently took Automodel's shape (contributors mount in sorted() order, so Automodel won). Layer 1 (the fix): add a NamespacedModel metaclass base. Pydantic freezes a model's JSON-schema name at class creation, so per-backend subclasses (AutomodelSchema / UnslothSchema / RlSchema) prefix each owned model there, emitting AutomodelTrainingSpec / UnslothTrainingSpec / RlDPOTraining, etc. Top-level names already starting with the prefix (AutomodelJobInput, RlJobInput, ...) are left unchanged. All three customization backends now namespace their own schemas, so the invariant holds as backends are added (without this, RL's OutputRequest/OutputResponse were correct in the spec only by accident — they won the bare name once Automodel/Unsloth vacated it). Layer 2 (backstop): tweak_spec / _normalize_refs_and_schema_keys gain a strict_collisions flag. It raises on a differing-content name collision so a future un-namespaced backend fails the build with an actionable message. A plugin opts in via [tool.nemo.openapi].strict_schema_collisions in its pyproject (only nemo-customizer does today, since its spec merges multiple backends into one app); every other spec keeps the legacy warn-and-collapse behavior, since the platform specs carry pre-existing collisions. While enabling the gate it surfaced a pre-existing duplicate: two identical BackendFormat enums (core-models vs nemo_platform_plugin.inference_middleware). Dedupe by having core-models re-export the plugin's canonical enum — the plugin is the lower-level package core-models already depends on, and IGW already treats it as canonical. Behavior-preserving; the platform spec is byte-identical. Known follow-ups (left as warnings, not fixed here; tracked in AALGO-352): DeleteResponse and GenericSortField each collide across common / core-entities / guardrails with genuinely different content. Regenerates plugins/nemo-customizer/openapi/openapi.yaml. Refs AALGO-351. Signed-off-by: Albert Cui <albcui@nvidia.com>
- Document that inheriting the namespaced base applies extra="forbid" uniformly across all RL models, including RlJobOutput/OutputResponse that previously defaulted to extra="ignore" (intentional: matches the automodel/unsloth backends, which rehydrate via the same model_validate(spec.model_dump()) path without carrying foreign fields). - Add a regression test pinning that a subclass declaring its own model_config still inherits extra="forbid" from the base. - Drop the redundant BackendFormat re-export comment; the reuse is evident from the import. Signed-off-by: Albert Cui <albcui@nvidia.com>
albcui
force-pushed
the
albcui/aalgo-351-customizer-openapi-schema-name-collision
branch
from
July 17, 2026 17:56
3976cb1 to
f7dc6fc
Compare
albcui
enabled auto-merge
July 17, 2026 19:29
albcui
deleted the
albcui/aalgo-351-customizer-openapi-schema-name-collision
branch
July 17, 2026 19:52
albcui
added a commit
that referenced
this pull request
Jul 20, 2026
#737 fixed the Automodel/Unsloth backends, but two collisions remained across the merged platform services and were silently collapsed by the schema-name normalizer: - DeleteResponse: core-entities' model (required id + deleted_count) collided with the shared nmp.common.api.common.DeleteResponse (id, deleted_at); guardrails had redefined the shared shape locally. - GenericSortField: core-entities, guardrails, and common each defined a different enum under the same name. The merge kept only the first-seen schema and repointed every $ref to it, so delete/sort endpoints across services referenced the wrong contract in the generated SDK. Give each its true name (schema-name change only; JSON wire format unchanged): - guardrails: drop the local DeleteResponse, import the byte-identical shared nmp.common.api.common.DeleteResponse. - core-entities: DeleteResponse -> EntityDeleteResponse (keeps deleted_count), updating all four delete endpoints. - core-entities GenericSortField -> WorkspaceSortField; guardrails GenericSortField -> GuardrailConfigSortField. Regenerates openapi/openapi.yaml and the ga/ merged specs. Refs AALGO-352. Signed-off-by: Albert Cui <albcui@nvidia.com>
albcui
added a commit
that referenced
this pull request
Jul 20, 2026
The differing-content collision gate added in #737 was opt-in: only the nemo-customizer plugin spec enforced it, while platform/service specs stayed on warn-and-collapse. That is exactly how the AALGO-352 collisions shipped silently. With those fixed (zero collisions across the platform, all services, and all plugins), flip the default so any future collision fails spec generation loudly instead of shipping a wrong SDK contract: - tweak_spec, apply_schema_fixes, and PluginConfig.strict_schema_collisions default to True. The plugin "lenient" branch now passes strict_collisions=False explicitly (it previously relied on the old default). - A spec can still opt out via strict_collisions=False / [tool.nemo.openapi].strict_schema_collisions = false. - Generalize the collision error message (rename/dedupe or namespace) and invert the unit tests to pin the policy: default raises, opt-out warns. Enforcement is at spec-generation time (make refresh-openapi / the manual openapi-generator pre-commit hook). CI does not regenerate the spec today, so this does not yet gate PRs on its own; adding a CI regen/drift check is a follow-up. Refs AALGO-352. Signed-off-by: Albert Cui <albcui@nvidia.com>
albcui
added a commit
that referenced
this pull request
Jul 20, 2026
#737 fixed the Automodel/Unsloth backends, but two collisions remained across the merged platform services and were silently collapsed by the schema-name normalizer: - DeleteResponse: core-entities' model (required id + deleted_count) collided with the shared nmp.common.api.common.DeleteResponse (id, deleted_at); guardrails had redefined the shared shape locally. - GenericSortField: core-entities, guardrails, and common each defined a different enum under the same name. The merge kept only the first-seen schema and repointed every $ref to it, so delete/sort endpoints across services referenced the wrong contract in the generated SDK. Give each its true name (schema-name change only; JSON wire format unchanged): - guardrails: drop the local DeleteResponse, import the byte-identical shared nmp.common.api.common.DeleteResponse. - core-entities: DeleteResponse -> EntityDeleteResponse (keeps deleted_count), updating all four delete endpoints. - core-entities GenericSortField -> WorkspaceSortField; guardrails GenericSortField -> GuardrailConfigSortField. Regenerates openapi/openapi.yaml and the ga/ merged specs. Refs AALGO-352. Signed-off-by: Albert Cui <albcui@nvidia.com>
albcui
added a commit
that referenced
this pull request
Jul 20, 2026
The differing-content collision gate added in #737 was opt-in: only the nemo-customizer plugin spec enforced it, while platform/service specs stayed on warn-and-collapse. That is exactly how the AALGO-352 collisions shipped silently. With those fixed (zero collisions across the platform, all services, and all plugins), flip the default so any future collision fails spec generation loudly instead of shipping a wrong SDK contract: - tweak_spec, apply_schema_fixes, and PluginConfig.strict_schema_collisions default to True. The plugin "lenient" branch now passes strict_collisions=False explicitly (it previously relied on the old default). - A spec can still opt out via strict_collisions=False / [tool.nemo.openapi].strict_schema_collisions = false. - Generalize the collision error message (rename/dedupe or namespace) and invert the unit tests to pin the policy: default raises, opt-out warns. Enforcement is at spec-generation time (make refresh-openapi / the manual openapi-generator pre-commit hook). CI does not regenerate the spec today, so this does not yet gate PRs on its own; adding a CI regen/drift check is a follow-up. Refs AALGO-352. Signed-off-by: Albert Cui <albcui@nvidia.com>
albcui
added a commit
that referenced
this pull request
Jul 20, 2026
#737 fixed the Automodel/Unsloth backends, but two collisions remained across the merged platform services and were silently collapsed by the schema-name normalizer: - DeleteResponse: core-entities' model (required id + deleted_count) collided with the shared nmp.common.api.common.DeleteResponse (id, deleted_at); guardrails had redefined the shared shape locally. - GenericSortField: core-entities, guardrails, and common each defined a different enum under the same name. The merge kept only the first-seen schema and repointed every $ref to it, so delete/sort endpoints across services referenced the wrong contract in the generated SDK. Give each its true name (schema-name change only; JSON wire format unchanged): - guardrails: drop the local DeleteResponse, import the byte-identical shared nmp.common.api.common.DeleteResponse. - core-entities: DeleteResponse -> EntityDeleteResponse (keeps deleted_count), updating all four delete endpoints. - core-entities GenericSortField -> WorkspaceSortField; guardrails GenericSortField -> GuardrailConfigSortField. Regenerates openapi/openapi.yaml and the ga/ merged specs. Refs AALGO-352. Signed-off-by: Albert Cui <albcui@nvidia.com>
albcui
added a commit
that referenced
this pull request
Jul 20, 2026
The differing-content collision gate added in #737 was opt-in: only the nemo-customizer plugin spec enforced it, while platform/service specs stayed on warn-and-collapse. That is exactly how the AALGO-352 collisions shipped silently. With those fixed (zero collisions across the platform, all services, and all plugins), flip the default so any future collision fails spec generation loudly instead of shipping a wrong SDK contract: - tweak_spec, apply_schema_fixes, and PluginConfig.strict_schema_collisions default to True. The plugin "lenient" branch now passes strict_collisions=False explicitly (it previously relied on the old default). - A spec can still opt out via strict_collisions=False / [tool.nemo.openapi].strict_schema_collisions = false. - Generalize the collision error message (rename/dedupe or namespace) and invert the unit tests to pin the policy: default raises, opt-out warns. Enforcement is at spec-generation time (make refresh-openapi / the manual openapi-generator pre-commit hook). CI does not regenerate the spec today, so this does not yet gate PRs on its own; adding a CI regen/drift check is a follow-up. Refs AALGO-352. Signed-off-by: Albert Cui <albcui@nvidia.com>
This was referenced Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automodel and Unsloth each define models named
TrainingSpec(plus LoRAParams, DatasetSpec, ScheduleSpec, BatchSpec, OptimizerSpec, OutputRequest, OutputResponse). Because both backends mount into one/apis/customizationFastAPI app, Pydantic's module-qualified$defskeys were collapsed by the platform's schema-name normalizer, so both*JobInput.trainingended up$ref-ing a singleTrainingSpec— Unsloth's request contract silently took Automodel's shape.Layer 1 (the fix): add a NamespacedModel metaclass base. Pydantic freezes a model's JSON-schema name at class creation, so per-backend subclasses (AutomodelSchema / UnslothSchema) prefix each owned model there, emitting AutomodelTrainingSpec / UnslothTrainingSpec. Top-level names already starting with the prefix (AutomodelJobInput, ...) are left unchanged.
Layer 2 (backstop):
tweak_spec/_normalize_refs_and_schema_keysgain astrict_collisionsflag. It raises on a differing-content name collision so a future un-namespaced backend fails the build with an actionable message. The flag is enabled only for the self-contained plugin specs (where the customization app lives); platform/service specs keep the legacy warn-and-collapse behavior, since they carry pre-existing collisions.While enabling the gate it surfaced a pre-existing duplicate: two identical BackendFormat enums (core-models vs nemo_platform_plugin.inference_middleware). Dedupe by having core-models re-export the plugin's canonical enum — the plugin is the lower-level package core-models already depends on, and IGW already treats it as canonical. Behavior-preserving; the platform spec is byte-identical.
Known follow-ups (left as warnings, not fixed here): DeleteResponse and GenericSortField each collide across common / core-entities / guardrails with genuinely different content.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
$refrewriting, and clearer warning/error behavior.Tests