feat(assets): add namespaced model_type tags and align tag semantics#14511
feat(assets): add namespaced model_type tags and align tag semantics#14511synap5e wants to merge 7 commits into
Conversation
Amp-Thread-ID: https://ampcode.com/threads/T-019ecf39-2e6f-747d-ae80-addba6b8e4f5 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019ecf39-2e6f-747d-ae80-addba6b8e4f5 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019ecf39-2e6f-747d-ae80-addba6b8e4f5 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019ecf39-2e6f-747d-ae80-addba6b8e4f5 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019ecf39-2e6f-747d-ae80-addba6b8e4f5 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019ecf39-2e6f-747d-ae80-addba6b8e4f5 Co-authored-by: Amp <amp@ampcode.com>
4a757fc to
4340337
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4340337c69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR makes asset tags case-sensitive throughout the stack. A new Alembic migration ( 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests-unit/assets_test/test_prune_orphaned_assets.py (1)
32-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope is no longer applied in
find_asset, making these assertions cross-test ambiguous.Line 32 drops scope-based narrowing, but callers still pass
scopeand use generic names (input.bin/output.bin). That can match unrelated assets and produce false pass/fail outcomes.Suggested hardening
def find_asset(http: requests.Session, api_base: str): """Query API for assets matching scope and optional name.""" - def _find(scope: str, name: str | None = None) -> list[dict]: + def _find(_scope: str, name: str | None = None) -> list[dict]: params = {"limit": "500"} if name: params["name_contains"] = name @@ def test_prune_across_multiple_roots( @@ scope = f"multi-{uuid.uuid4().hex[:6]}" - input_fp = create_seed_file("input", scope, "input.bin") - create_seed_file("output", scope, "output.bin") + input_name = f"{scope}-input.bin" + output_name = f"{scope}-output.bin" + input_fp = create_seed_file("input", scope, input_name) + create_seed_file("output", scope, output_name) trigger_sync_seed_assets(http, api_base) - assert find_asset(scope, input_fp.name) - assert find_asset(scope, "output.bin") + assert find_asset(scope, input_name) + assert find_asset(scope, output_name) @@ - assert not find_asset(scope, input_fp.name) - assert find_asset(scope, "output.bin") + assert not find_asset(scope, input_name) + assert find_asset(scope, output_name)Also applies to: 115-122
🤖 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 `@tests-unit/assets_test/test_prune_orphaned_assets.py` around lines 32 - 40, The find_asset function no longer applies scope-based filtering when retrieving assets, which causes test assertions to be ambiguous when multiple tests use generic asset names like input.bin or output.bin. Modify the find_asset function to accept and apply scope as a query parameter in the API request (similar to how name_contains is currently handled), ensuring that the returned assets are filtered by both scope and name to prevent false matches across different test scopes.
🧹 Nitpick comments (1)
alembic_db/versions/0005_allow_case_sensitive_tags.py (1)
23-33: ⚡ Quick winSQLite foreign key handling may leave FKs disabled on error.
If any statement between
PRAGMA foreign_keys=OFF(line 23) andPRAGMA foreign_keys=ON(line 33) raises an exception, foreign keys remain disabled for the connection. Consider wrapping in a try/finally or using a transaction to ensure cleanup.🛡️ Suggested safer pattern
if bind.dialect.name == "sqlite": - op.execute("PRAGMA foreign_keys=OFF") - op.execute( - "CREATE TABLE tags_new (" - "name VARCHAR(512) NOT NULL, " - "CONSTRAINT pk_tags PRIMARY KEY (name)" - ")" - ) - op.execute("INSERT INTO tags_new(name) SELECT name FROM tags") - op.execute("DROP TABLE tags") - op.execute("ALTER TABLE tags_new RENAME TO tags") - op.execute("PRAGMA foreign_keys=ON") - return + try: + op.execute("PRAGMA foreign_keys=OFF") + op.execute( + "CREATE TABLE tags_new (" + "name VARCHAR(512) NOT NULL, " + "CONSTRAINT pk_tags PRIMARY KEY (name)" + ")" + ) + op.execute("INSERT INTO tags_new(name) SELECT name FROM tags") + op.execute("DROP TABLE tags") + op.execute("ALTER TABLE tags_new RENAME TO tags") + finally: + op.execute("PRAGMA foreign_keys=ON") + return🤖 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 `@alembic_db/versions/0005_allow_case_sensitive_tags.py` around lines 23 - 33, The migration code disables foreign keys at the start with `PRAGMA foreign_keys=OFF` but if any of the subsequent `op.execute()` calls (such as CREATE TABLE tags_new, INSERT INTO tags_new, DROP TABLE tags, or ALTER TABLE tags_new RENAME) raise an exception, the final `PRAGMA foreign_keys=ON` statement will not execute, leaving foreign keys permanently disabled for the connection. Wrap the middle statements (the table creation, data insertion, and table swap operations) in a try/finally block to ensure that `PRAGMA foreign_keys=ON` is always executed regardless of whether an error occurs during the migration steps.
🤖 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 `@alembic_db/versions/0005_allow_case_sensitive_tags.py`:
- Around line 42-51: The downgrade path uses SQLite-specific SQL syntax that
will fail on PostgreSQL and MySQL databases. The operations in the downgrade
function including INSERT OR IGNORE and rowid references are not
database-agnostic. Wrap these SQLite-specific op.execute calls (the INSERT OR
IGNORE statement and the DELETE with rowid usage) in a conditional check for
SQLite dialect similar to the dialect check pattern used elsewhere in the
migration file, or rewrite the operations using portable SQL syntax that handles
both INSERT OR IGNORE (for SQLite), INSERT ... ON CONFLICT DO NOTHING (for
PostgreSQL), and INSERT IGNORE (for MySQL), and replace rowid references with
database-appropriate alternatives like ctid for PostgreSQL or proper primary key
handling.
---
Outside diff comments:
In `@tests-unit/assets_test/test_prune_orphaned_assets.py`:
- Around line 32-40: The find_asset function no longer applies scope-based
filtering when retrieving assets, which causes test assertions to be ambiguous
when multiple tests use generic asset names like input.bin or output.bin. Modify
the find_asset function to accept and apply scope as a query parameter in the
API request (similar to how name_contains is currently handled), ensuring that
the returned assets are filtered by both scope and name to prevent false matches
across different test scopes.
---
Nitpick comments:
In `@alembic_db/versions/0005_allow_case_sensitive_tags.py`:
- Around line 23-33: The migration code disables foreign keys at the start with
`PRAGMA foreign_keys=OFF` but if any of the subsequent `op.execute()` calls
(such as CREATE TABLE tags_new, INSERT INTO tags_new, DROP TABLE tags, or ALTER
TABLE tags_new RENAME) raise an exception, the final `PRAGMA foreign_keys=ON`
statement will not execute, leaving foreign keys permanently disabled for the
connection. Wrap the middle statements (the table creation, data insertion, and
table swap operations) in a try/finally block to ensure that `PRAGMA
foreign_keys=ON` is always executed regardless of whether an error occurs during
the migration steps.
🪄 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: Pro Plus
Run ID: c1776315-8449-45c8-acf4-3f40bb5ffd5b
📒 Files selected for processing (26)
alembic_db/versions/0005_allow_case_sensitive_tags.pyapp/assets/api/routes.pyapp/assets/api/schemas_in.pyapp/assets/api/upload.pyapp/assets/database/queries/tags.pyapp/assets/helpers.pyapp/assets/services/ingest.pyapp/assets/services/path_utils.pycomfy_api/feature_flags.pyopenapi.yamlserver.pytests-unit/assets_test/conftest.pytests-unit/assets_test/queries/test_asset_info.pytests-unit/assets_test/queries/test_tags.pytests-unit/assets_test/services/test_path_utils.pytests-unit/assets_test/test_assets_missing_sync.pytests-unit/assets_test/test_crud.pytests-unit/assets_test/test_downloads.pytests-unit/assets_test/test_list_cursor.pytests-unit/assets_test/test_list_filter.pytests-unit/assets_test/test_metadata_filters.pytests-unit/assets_test/test_prune_orphaned_assets.pytests-unit/assets_test/test_tags_api.pytests-unit/assets_test/test_uploads.pytests-unit/feature_flags_test.pytests-unit/websocket_feature_flags_test.py
Amp-Thread-ID: https://ampcode.com/threads/T-019ecf39-2e6f-747d-ae80-addba6b8e4f5 Co-authored-by: Amp <amp@ampcode.com>
Introduce
model_type:namespaced tag, align asset scanner to set this based on the registered model folder**(s)** the asset belongs to. Drop deep folder-based tag construction, and placement logic. Update asset-upload behaviour to use key system/known tags and faithfully store the rest without affecting on-disk location.Note: This PR does not attempt to migrate assets tagged by the previous version of the asset system. This migration is feasible, and if desired could be landed in a followup PR.
What this does
Reworks asset tags so they are no longer overloaded as model classification, upload routing, and filesystem subdirectory instructions at the same time.
This PR makes three related behavior changes:
models + model_type:<folder_name>instead of flat labels such asmodels + checkpointsormodels + loras.input,output, ormodels), model uploads use exactly onemodel_type:<folder_name>, and nested placement uses the explicitsubfolderfield. Extra tags are labels only.The new backend-generated model classification shape is:
models: the asset has a real filesystem path under at least one allowed registered Comfy model folder.model_type:<folder_name>: the asset path is under the registered model folder named<folder_name>, preserving the registered folder name casing.Examples:
models + model_type:checkpoints,models + model_type:loras,models + model_type:LLM.Backend-generated classification is added only from trusted filesystem facts, such as a scanner-discovered path, an already-written
/upload/imagefile, or the final destination of a multipart byte upload. Tag filters still operate on the single persisted tag set, so once a tag exists it is filterable regardless of whether it originated from a caller or from backend classification.Motivation
The old asset tag behavior was not just underspecified; it produced wrong or misleading results for real Comfy model configurations.
Extra model paths were flattened into ambiguous path labels. Comfy model folders are defined by
folder_paths.folder_names_and_paths, and a single model type can have registered paths outside the stock<models>/<folder>layout. The old path-derived tags treated the registered folder name and parent path fragments as ordinary flat tags such ascheckpoints,loras, or arbitrary subfolder names. That meant the API did not have a stable way to say “this asset belongs to the registeredcheckpointsmodel type” versus “this asset merely has a caller/path label namedcheckpoints.” Namespaced tags make the model type explicit:models + model_type:<folder_name>.Registered non-model folders could be treated like model upload destinations.
folder_names_and_pathsalso contains entries that are not safe model upload targets. In particular,custom_nodesandconfigsshould not become places where asset upload requests can write user bytes just because they are registered folder names. This PR excludes those names from model classification/upload destination resolution.Overlapping roots made one flat classification insufficient. A path can belong to more than one relevant backend fact, for example a registered model folder under the output root. The old “choose one root and then emit path fragments” shape could lose part of the truth or encode it as misleading directory labels. This PR lets backend classification keep each trusted fact as tags, e.g.
output + models + model_type:*when both root and model-folder membership are true.Upload routing and tag labels were conflated. Multipart upload tags previously doubled as routing, model category, and subdirectory path components. That made
models + checkpoints + foo + barboth a classifier and a filesystem placement instruction. This PR keeps routing narrow: new byte uploads use exactly one destination role (input,models, oroutput), model uploads use exactly onemodel_type:<folder_name>, and all other tags remain labels.This PR also changes the location semantics for new multipart
/api/assetsbyte uploads. Previously, ordered tags described the destination root, model category, and nested filesystem subdirectories: for example,input + foowrote underinput/foo, andmodels + checkpoints + foo + barwrote under a checkpoints folder plusfoo/bar. In this PR, multipart upload location is selected by explicit placement fields: one destination role (input,output, ormodels), for model uploads exactly onemodel_type:<folder_name>, and an optionalsubfolderform field for nested placement. Extra tags are labels only and do not create subdirectories.That location change is specific to multipart
/api/assetsbyte uploads./upload/imagekeeps its existing placement semantics: it still usestypeandsubfolderform fields to choose where the image is written. Known/upload/imagesemantic subfolders such aspasted,painter,webcam,threed, and3dare mirrored as asset tags when the already-written file is registered.The resulting model is intentionally simple: tag filters operate on persisted labels, while backend-generated model classification is added only from trusted filesystem facts from scan/register/upload placement.
Examples
File under a registered checkpoint folder:
models,model_type:checkpointsinclude_tags=models,model_type:checkpointsFile under a case-preserving registered model folder named
LLM:models,model_type:LLMmodel_type:LLMandmodel_type:llmare distinct tagsMultipart byte upload with tags
models + model_type:checkpoints:uploaded,models, andmodel_type:checkpointsMultipart byte upload with tags
input + model_type:checkpoints:model_type:checkpointsas a labelMultipart byte upload with tags
models + model_type:checkpoints + foo + barand nosubfolder:fooandbaras labelsfoo/barsubdirectoriesMultipart byte upload with tags
models + model_type:checkpointsandsubfolder=foo/bar:foo/bar/<digest><ext>subfolderfield, not from tags/api/assets/from-hashwith tagsmodels + model_type:checkpoints:/upload/imagewithtype=input&subfolder=pasted:input/pasted/<filename>using the existing image-upload fieldsinput,uploaded, and the known semanticpastedtagChanges
tags, with a downgrade path that lowercases/merges mixed-case tags before restoring the old constraint.inputoutputtempmodelsmodel_type:<folder_name>checkpoints,loras, or arbitrary parent directory names from model paths.input,models, oroutput.model_type:<folder_name>tag for new multipart model uploads.subfoldersupport for nested placement.configsandcustom_nodesas model upload destinations.uploadedto successful byte upload registrations, including already-written/upload/imageregistrations./upload/imagesubfolders (pasted,painter,webcam,threed,3d) as tags when registering those files as assets.uploaded, or copy path-derived classification.file_path/display_nameresponse locator fields to feat(assets) Add asset file_path and display_name response fields #14005, which can build on these tag and upload semantics.supports_model_type_tags: truein server feature flags for frontend capability detection.Behavior / compatibility notes
modelstag matchesinclude_tags=modelsregardless of whether it was supplied by a caller or generated from a trusted path.model_type:<folder_name>to a filesystem-backed model asset, that follow-up moves/re-registers the file so the label and loader-visible location stay coherent.output/foo/bar.pngis classified asoutput, notoutput + foo.subfolderform field./upload/imageplacement is not changed: it continues to use itstypeandsubfolderform fields rather than asset tags. Known image-upload subfolder names are also stored as tags for filtering.checkpointsare not migrated intomodel_type:checkpoints; clients that need transition compatibility should handle both shapes.output + models + model_type:*; persisted tags are not dynamically recomputed at response time.Verification
python3 -m py_compileon touched Python files and the migration.uv run --with pytest --with sqlalchemy --with pydantic --with aiohttp --with requests --with filelock --with pyyaml --with pydantic-settings --with pillow --with blake3 pytest -q tests-unit/assets_test/services/test_ingest.py tests-unit/assets_test/services/test_path_utils.py tests-unit/assets_test/queries/test_asset_info.py::TestListReferencesPage39 passeduv run --with pytest --with sqlalchemy --with pydantic --with aiohttp --with requests --with filelock --with pyyaml --with pydantic-settings --with pillow --with blake3 pytest -q tests-unit/assets_test/services/test_path_utils.py18 passeduv run --with-requirements requirements.txt --with pytest pytest -q tests-unit/assets_test/test_uploads.py31 passeduv run --with-requirements requirements.txt --with-requirements tests-unit/requirements.txt pytest -q tests-unit --tb=short902 passed, 10 skipped, 9 warningsuv run --with-requirements requirements.txt --with-requirements tests-unit/requirements.txt pytest -q tests-unit/feature_flags_test.py tests-unit/websocket_feature_flags_test.py --tb=short23 passedupgrade headaccepts mixed-case tags such asNewTagandmodel_type:LLM0004_drop_tag_typerestores the lowercase constraint after normalizing mixed-case tags