fix(BA-6015): persist resolved model-definition path when set None from request#11638
Open
seedspirit wants to merge 3 commits into
Open
fix(BA-6015): persist resolved model-definition path when set None from request#11638seedspirit wants to merge 3 commits into
seedspirit wants to merge 3 commits into
Conversation
github-actions
Bot
added
size:L
seedspirit
added a commit
that referenced
this pull request
May 15, 2026
seedspirit
changed the title
seedspirit
changed the title
Open
3 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes BA-6015 by ensuring the resolved model_definition_path (auto-detected from the model vfolder or explicitly overridden) is written back into the merged RevisionDraft, so the persisted deployment revision reflects the actual mounted model-definition file.
Changes:
- Fold mount identity (
MountMetadata) intoRevisionDraftand make it participate in the merge chain (including persistence and spec projection). - Introduce
FetchedModelDefinition/FetchedConfigFileto propagate the matched vfolder filename back into the draft. - Update legacy/v2 revision creation flows and add unit tests to verify resolved path persistence and merge semantics.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/manager/sokovan/deployment/test_revision_draft_reader.py | Adds coverage ensuring vfolder-resolved model-definition path is recorded into merged mounts. |
| tests/unit/manager/data/deployment/test_revision_draft_merge.py | Adds coverage for model_definition_path “None means fallback” merge behavior. |
| src/ai/backend/manager/sokovan/deployment/revision_draft/reader.py | Uses request_draft.mounts as the mount context and writes resolved definition path back into a draft layer. |
| src/ai/backend/manager/sokovan/deployment/deployment_controller.py | Removes sidecar mounts arg and persists mounts from the merged draft, including resolved definition path. |
| src/ai/backend/manager/services/model_serving/services/model_serving.py | Adapts legacy ModifyEndpoint flow to the new mounts-in-draft merge/persist behavior. |
| src/ai/backend/manager/repositories/deployment/storage_source/storage_source.py | Returns matched config filename/payload wrapper and returns FetchedModelDefinition with matched path. |
| src/ai/backend/manager/repositories/deployment/repository.py | Adjusts fetch-model-definition typing/docs to return FetchedModelDefinition. |
| src/ai/backend/manager/data/deployment/types.py | Adds FetchedModelDefinition, adds mounts to RevisionDraft, adds _merge_mounts, and updates spec projection. |
| src/ai/backend/manager/data/deployment/creator.py | Renames projection to to_draft_with_extra_mount to inject permission-resolved extra mounts into draft mounts. |
| changes/11638.fix.md | Towncrier entry for the fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
seedspirit
commented
May 18, 2026
| preset_values: list[PresetValueData] = field(default_factory=list) | ||
|
|
||
| def to_draft(self) -> RevisionDraft: | ||
| def to_draft_with_extra_mount(self, extra_mounts: list[MountInfoEntry]) -> RevisionDraft: |
Contributor
Author
There was a problem hiding this comment.
Used in DeploymentController. This has been updated to include mount information in RevisionDraft. Structural improvements are needed going forward.
Mount identity (model vfolder, definition path, destination, extra mounts, subpath) now travels inside RevisionDraft and participates in the merge chain instead of being threaded as a sidecar argument through add_revision and the draft readers. The vfolder scan records the matched candidate path back into the merged draft, so the resolved model-definition path is persisted on the revision instead of the request's possibly-empty value. ModelRevisionCreator.to_draft is renamed to to_draft_with_extra_mount and now derives the rest of the mount metadata itself, taking only the permission-resolved extra mounts. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…missing in merge Pydantic now rejects empty strings at the DTO boundary via min_length=1, and _merge_mounts uses a truthy check so any empty string that bypasses the DTO no longer clobbers a resolved lower-priority model_definition_path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
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.
Problem
When a request omits
model_definition_path, the model service revision was persisted with the request's empty /Nonevalue instead of the model-definition file actually resolved from the model vfolder. The resolved path was used at runtime but never written back, so the stored revision did not reflect what was really mounted.Fix
The vfolder scan now records the matched candidate path (
model-definition.yaml/.yml, or an explicit override) back into the merged draft, so the resolvedmodel_definition_pathis persisted on the revision instead of the empty input value.How (mechanism)
To do this cleanly, mount identity (model vfolder, definition path, destination, extra mounts, subpath) is folded into
RevisionDraftand participates in the merge chain, instead of being threaded as a separate sidecarmountsargument throughadd_revisionand the draft readers:FetchedModelDefinition/FetchedConfigFiletypes carry the matched filename out of the storage source so the resolved path can flow back into the draft._merge_mountsuses upper-priority semantics withNone-fallback formodel_definition_path/vfolder_subpath; every other mount field is sourced from the samerequest_draft.mounts, so the merge cannot tangle values.ModelRevisionCreator.to_draft→to_draft_with_extra_mount: the creator derives the fullMountMetadatafromself.mountsand only takes the permission-resolvedextra_mounts.Test plan
pants test tests/unit/manager/data/deployment:: tests/unit/manager/sokovan/deployment::model_definition_path: resolvedmodel-definition.ymlpath is persisted on the revisionmodel_definition_pathoverride on modify takes precedence over the resolved/stored pathResolves BA-6015