feat(assets): enable asset system by default; remove --enable-assets#14699
feat(assets): enable asset system by default; remove --enable-assets#14699mattmillerai wants to merge 6 commits into
Conversation
The asset system is now always on. Removes the --enable-assets opt-in flag and the conditional registration it gated: - Asset API routes are always registered (with the user manager) - The background asset scanner always runs (hashing stays opt-in via --enable-asset-hashing, default off — no new blake3 cost on a default launch) - /upload/image asset registration and output enrichment always active - The "assets" server feature flag is now unconditionally true - A database-init failure degrades gracefully instead of hard-exiting, matching the prior default-path behavior
mattmillerai
requested review from
Kosinkadink,
alexisrolland,
comfyanonymous,
guill,
kijai and
rattus128
as code owners
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR removes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
…rom-oss-assets-always-on-cloud
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 8 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 4 |
| 🟢 Low | 1 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…rom-oss-assets-always-on-cloud
…ts as no-op Address automated review feedback on the always-on assets change: - setup_database(): on DB init failure or missing dependencies, call disable_assets_routes() + asset_seeder.disable() so /api/assets/* returns a clean 503 instead of 500s against an uninitialized DB (restores the fail-safe the removed else-branch used to provide). - feature_flags: derive the advertised 'assets' capability from live backend availability instead of hardcoding True, so clients degrade gracefully. - cli_args: re-add --enable-assets as a hidden deprecated no-op so existing launchers still passing the flag don't abort argparse. - routes: add assets_enabled() accessor for the feature-flag derivation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
main.py (1)
463-468: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilent fallback when DB dependencies are missing.
The
elsebranch correctly fails safe by callingdisable_assets_routes()andasset_seeder.disable(), but nothing is logged here — unlike theexceptbranch below (Line 481), which does log the failure. An operator with missing optional DB deps will see assets silently disabled with no startup message explaining why.📝 Add a log line for visibility
else: # Optional DB dependencies are missing, so init_db() is skipped and the # asset backend has no database. Disable assets so /api/assets/* returns # a clean 503 instead of 500s against an uninitialized DB. + logging.warning("Optional database dependencies are missing; assets system disabled.") disable_assets_routes() asset_seeder.disable()🤖 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 `@main.py` around lines 463 - 468, The optional-DB fallback in the `else` branch silently disables assets without any startup visibility, unlike the `except` path. Add a log message in the `disable_assets_routes()` / `asset_seeder.disable()` branch in `main.py` that explains the database dependencies are missing and assets are being disabled, using the same logger pattern as the nearby startup/error handling so operators can tell why this happened.
🤖 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.
Nitpick comments:
In `@main.py`:
- Around line 463-468: The optional-DB fallback in the `else` branch silently
disables assets without any startup visibility, unlike the `except` path. Add a
log message in the `disable_assets_routes()` / `asset_seeder.disable()` branch
in `main.py` that explains the database dependencies are missing and assets are
being disabled, using the same logger pattern as the nearby startup/error
handling so operators can tell why this happened.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b93bcaaa-dd26-40d7-872c-0434fd94ad3f
📒 Files selected for processing (4)
app/assets/api/routes.pycomfy/cli_args.pycomfy_api/feature_flags.pymain.py
…-remove-enable-assets-from-oss-assets-always-on-cloud
|
Addressed the CodeRabbit nitpick: the missing-optional-DB-deps fallback in Note on CI: the |
Addresses CodeRabbit review: the missing-optional-dependency fallback in setup_database() silently disabled the assets system with no startup message, unlike the exception path which logs. Add a warning so operators can tell why /api/assets/* returns 503.
mattmillerai
force-pushed
the
matt/be-786-phase-1-remove-enable-assets-from-oss-assets-always-on-cloud
branch
from
37acd25 to
d224df7
Compare
2 participants
ELI-5
ComfyUI's asset system — the local database that tracks your models / inputs / outputs and serves the
/api/assetsroutes — used to be hidden behind an opt-in--enable-assetsflag. This turns it on for everyone by default and deletes the flag. The potentially slow part (content hashing on large model folders) stays opt-in via--enable-asset-hashing(off by default), so a normal startup costs the same as before.What changed
--enable-assetsfromcomfy/cli_args.py. The asset system runs unconditionally on every startup.--enable-asset-hashing, so there is no new blake3 cost on a default launch./upload/imagealways attempts to register the upload as an asset, and output enrichment (enrich_output_with_assets) always runs.assetsserver feature flag is now unconditionallytrue.--enable-assetshard-exit.Note for users
--enable-assetsis removed, not kept as a no-op. Launch scripts or configs passing--enable-assetsshould drop it — the system is now always on. If a transitional deprecation shim is preferred over a hard removal, I'm happy to add one.Tests
tests-unit/execution_test/test_enrich_output.pysuite passes (13 tests).tests-unit/assets_test/conftest.pyno longer passes--enable-assetswhen launching the test server.