feat(assets) Add asset file_path and display_name response fields#14005
feat(assets) Add asset file_path and display_name response fields#14005synap5e wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds AssetRoot typing and new path utilities (compute_asset_response_paths, compute_file_path, compute_display_name, get_asset_root_folder_name_and_filepath). Routes now import and use compute_asset_response_paths to populate Asset responses' file_path and display_name (or set them to None). Upload validation for "models" tags now uses is_comfy_model_folder_name. Asset schema and OpenAPI mark name deprecated and add nullable file_path and display_name. Unit and upload tests were added/updated to cover the new behavior. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Heads up on the Two cleaner options:
(1) is the smaller change and avoids introducing dual conventions in the same file. |
| `bucket` is only set for model assets. The returned relative path always | ||
| uses `/` separators and is empty when the path is exactly the matched root. |
There was a problem hiding this comment.
I think this function relies on a flawed assumption that all models will be in the models directory. As an example, the built-in SaveCheckpoint, SaveLora, etc. models actually save to the outputs directory but are still loadable as models.
If possible, I would really like to avoid special-casing models as that's usually going to fail for edge cases. Could you explain a bit more about why we actually need the bucket as a first-class concept?
There was a problem hiding this comment.
New direction for this PR resolves this IMO.
file_path now represents the {asset's membership to} and {file location relative to} the asset's "storage root" i.e. input output temp models.
The prior special casing was a reaction to the edge cases, primarily
- overlapping model_folders via custom nodes e.g. a custom node registers a
unet_ggufoverlapping withcheckpoints extra_model_paths.yml
I have intentionally deferred extra_model_paths.yml models from this PR, but I think this is also solvable.
#14511 solves the categorisation and (non-extra-models) overlap issues rather than sneaking that into file_path
| def get_asset_category_and_relative_path( | ||
| file_path: str, | ||
| ) -> tuple[Literal["input", "output", "temp", "models"], str]: | ||
| ) -> tuple[RootCategory, str]: |
There was a problem hiding this comment.
I know this isn't actually new to this PR, but I'm a little concerned about the assumption that models will be exactly one of these. We should make sure this works for cases like the built-in SaveCheckpoint or SaveLora nodes where the model is located under the outputs directory, but should also be treated as a model.
There was a problem hiding this comment.
Under new PR direction, SaveCheckpoint to output/checkpoints should now remain output/checkpoints/foo.safetensors - if that folder is a registered model root then it should gain the appropriate tags ouput models model_type:... per #14511
Making SaveCheckpoint ouputs automatically register as models regardless of location could be a followup PR, but is currently blocked by internal model loading behaviour. Once asset system node API lands, this should unblock.
73ba8c6 to
c533a88
Compare
|
@guill @synap5e — picked this up to get it unblocked. Where it stands: On the model-path concern (@guill) — let me be precise here, since my first pass overstated it: The redesign does remove the "models live in the What it does not do is auto-reclassify an unregistered Tests: Conflicts + fixes (@synap5e):
CI is green apart from the now-fixed Windows job. @synap5e — flag if you'd rather I revert any of the above; otherwise this just needs a maintainer approval. |
|
Thanks Matt — I picked this back up and changed direction after lining it up with #14511. The branch is now rebuilt on top of the namespaced tag/upload semantics from #14511. I also kept the important cleanup from your earlier pass: no hand-edited The bigger semantic change is that
That separation is what resolves Guill's concern about output-backed model folders. A file can be physically located under For arbitrary external registered model roots, this PR currently omits So the revised split is:
|
aa376ac to
61ae5ff
Compare
ed2016a to
733b4eb
Compare
4a757fc to
4340337
Compare
Amp-Thread-ID: https://ampcode.com/threads/T-019eddf6-336e-71db-b94b-655afe52505e Co-authored-by: Amp <amp@ampcode.com>
733b4eb to
990242a
Compare
Add optional
file_pathanddisplay_nameto asset responses. Fields are constructed using known asset roots (input,output,temp, registeredmodelsfolders).Note External
extra_model_paths.yamlare a known gap - models discovered here do not get these fields. Followup is planned.What this does
Adds two optional fields to the asset API's
AssetandAssetCreatedresponse schemas:file_path: A runtime-computed storage namespace locator for filesystem-backed asset references. It is not an absolute filesystem path, not a unique identity, and not a model-loader path.display_name: A human-facing label derived fromfile_path, usually the path below the top-level storage namespace.These fields are computed late from the stored absolute
AssetReference.file_pathwhen building API responses. They are not persisted separately and are not derived from tags.This PR is now stacked on / aligned with #14511's namespaced asset tag semantics. Tags answer “what can this asset load as or be filtered by?”;
file_pathanswers “where does this asset reference live in Comfy's storage namespace?”. Recommend reviewing this stack as a pair.Motivation
In asset mode, the asset response's
namefield is overloaded. Depending on the asset's origin, clients may treat the same field as:The frontend currently has to infer intent from value shape, tags, or backend-specific heuristics. This PR separates those roles:
id: API identity.name: existing reference label, deprecated for path/display semantics.file_path: storage locator for file-backed references.display_name: UI label derived from the storage locator.tags: classification/filter labels, including backend-generated model/loadability facts such asmodelsandmodel_type:<folder_name>.Semantics
file_pathis a storage namespace locator, not a model-loader namespace.Built-in storage roots:
input/<relative_path>output/<relative_path>temp/<relative_path>models/<relative_path_under_models_dir>Model loadability/classification remains represented by tags from #14511:
modelsmodel_type:<folder_name>This avoids conflating physical storage layout with model type. For example, a file under the legacy physical folder
models/unetremainsmodels/unet/...infile_path, while tags can say it is loadable asmodel_type:diffusion_models.The response fields are omitted for hash-only/from-hash references or paths outside known storage roots.
Examples
Input file
Physical path:
Response:
Output image
Physical path:
Response:
Stock checkpoint file
Physical path:
Response:
Relevant tags:
Legacy physical diffusion-model folder
Physical path:
Response:
Relevant tags:
Output-backed registered model folder
If
<output>/checkpointsis registered as acheckpointsmodel folder, a file at:still has a storage locator under output:
Relevant tags expose both facts:
Ordinary output checkpoint-like file
If
<output>/checkpointsis not registered as a model folder:Relevant tags:
Hash-only/from-hash reference
A reference created from an existing hash can have user labels that look like system tags:
{ "tags": ["models", "model_type:checkpoints"] }but because it has no trusted
AssetReference.file_path, the response omitsfile_pathanddisplay_name.Extra model paths
This PR deliberately does not rewrite arbitrary external registered model folders to
models/<registered_model_folder>/....Comfy currently flattens
extra_model_paths.yamlentries intofolder_paths.folder_names_and_paths, losing the YAML source key / provenance needed to build an unambiguous public storage locator. Until that provenance is preserved, external registered roots still receive model classification tags but omitfile_path/display_name.A future follow-up can add an
extra_models/<source_alias>/<relative_path>namespace once extra-model-path provenance is recorded.Changes
file_pathanddisplay_nametoAsset, inherited byAssetCreated.compute_asset_response_pathscompute_file_pathcompute_display_nameexclude_none=Trueresponse behavior.models + model_type:<folder_name>subfolderopenapi.yamlhand edits out; core's API spec is downstream of the cloud spec sync.Verification
uv run --with pytest pytest tests-unit/assets_test/services/test_path_utils.py -q # 39 passeduv run --with pytest --with pytest-asyncio pytest tests-unit/assets_test/test_uploads.py -q # 34 passeduv run --with-requirements requirements.txt --with-requirements tests-unit/requirements.txt pytest -q tests-unit/assets_test # 442 passed, 10 skipped