refactor(cql): remove dead normalize() family from loader.py - #879
mattmillerai wants to merge 1 commit into
Conversation
The load_graph removal left loader.py's normalize() and its helpers (_looks_like_object_info, _looks_like_api_workflow, _from_object_info, _normalize_input, _from_api_workflow) reachable only from their own tests. No production code calls normalize: the engine builds its Graph from raw object_info directly, resilient_load_object_info is the only symbol the rest of the package imports from this module, and normalize was never in cql.__all__. Delete the normalize family and the now-unused CQLRuntimeError import, rewrite the module docstring to describe only the resilient object_info cache/fetch wrapper that remains, and delete tests/comfy_cli/cql/test_loader.py (every test in it exercised normalize). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe loader now provides resilient loading without graph normalization. The ChangesLoader normalization removal
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The change removes unused normalization code and its dedicated tests; no current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
mattmillerai
added
agent-coded
![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 1 finding(s).
| Severity | Count |
|---|---|
| ⚪ Nit | 1 |
Panel: 6/6 reviewers contributed findings.
| This module is the resilient object_info cache/fetch wrapper. | ||
| ``resilient_load_object_info`` wraps the engine's loaders | ||
| (``comfy_cli.cql.engine._load_from_file`` / ``_load_from_target``) with a | ||
| cache-first TTL gate, auto-caches every successful fetch per host, retries |
There was a problem hiding this comment.
⚪ Nit — The new docstring describes the wrapper as applying a "cache-first TTL gate" unconditionally, but resilient_load_object_info only consults read_fresh_object_info_cache when mode == "cloud"; local targets always fetch live. Qualifying this as cloud-only avoids a future change "fixing" local to match the docstring and hiding newly installed custom nodes for the TTL window. Raised by 1 of 6 reviewers (claude-opus-5-thinking-max edge-case).
1 participant
ELI-5
There was a chunk of code in
loader.pycallednormalize()(plus five helperfunctions) whose whole job was to reshape different graph formats into one
common shape. A recent change removed the only real feature that used it, so now
nothing in the app calls it anymore — the only things keeping it "alive" were its
own tests. This PR deletes that dead code and its test file so the module is
smaller and only does the one job it still has: fetching and caching the node
catalog (
object_info) resiliently.What changed
comfy_cli/cql/loader.py— deletednormalizeand its helpers(
_looks_like_object_info,_looks_like_api_workflow,_from_object_info,_normalize_input,_from_api_workflow) and the# ---- normalization ----divider. Dropped the now-unused
CQLRuntimeErrorimport (it was raised onlyinside
normalize; the resilient path catchesLoadError, imported locally).Rewrote the module docstring so it describes only the resilient
object_info cache/fetch wrapper that remains.
tests/comfy_cli/cql/test_loader.py— deleted entirely; all five testsexercised
normalize, and itsOBJECT_INFO/API_WORKFLOWfixtures werelocal to the file (no other test imports them).
normalizewas module-public but was never exported incql.__all__andnever advertised, so this removes no documented API.
Why this is safe (dead-code verification)
Run before deleting:
grep -rn "normalize" --include='*.py' comfy_cli/— the onlycql/hits wereloader.py's own definitions. Every other hit is an unrelated symbol(
file_utils._normalize_path,billing.normalize_plans,workflow_to_api._normalize_combo_values,cm_cli_util.normalize_cm_cli_exit_code,etc.) that never calls
loader.normalize.normalizeor any of its helpers anywhere in the tree;the only importer was
tests/comfy_cli/cql/test_loader.py(deleted). The enginebuilds its
Graphfrom rawobject_infodirectly and never touches this module'sreshaping code; the only symbol the rest of the package imports from
loader.pyis
resilient_load_object_info.After deleting:
grep -rnE "normalize\b" --include='*.py' comfy_cli/cql/→ zero hits.Verification
ruff check .— passes (confirms no import became unused;timeis retainedbecause the resilient cache path still uses it).
ruff format --check .— passes.pytestfull suite — 7548 passed, 38 skipped, 4 failed. All 4 failures arepre-existing and environment-dependent, unrelated to this change (confirmed by
reproducing them identically on a clean checkout of the base branch): two umask
default-mode assertions in
test_file_utils.py, onepackaging-version-parsingassertion in
test_node_deps.py, and one console-line-wrap wording assertion intest_logs.py. None touchcql/loader/normalize.pytest tests/comfy_cli/cql/in isolation — 517 passed.Residual
test_loader_resilient.py→test_loader.py"for tidiness (optional; skip ifit churns the diff)." Skipped to keep this diff a clean pure-deletion; the
resilient/TTL tests keep their current filenames. A follow-up could do the
rename if a reviewer prefers it.
environment-dependent test failures listed above already fail on the base branch
in this environment. They are unrelated to this change and left untouched; worth
their own look if the CI environment hits the same umask/
packaging/console-widthconditions.
Provenance
ruff check .,ruff format --check ., andpytest(full suite +isolated
cql/run) as described above; dead-code claim verified by the grepsabove plus an
__all__check.diff a pure deletion — noted under Residual.