Repository navigation
Conversation
There was a problem hiding this comment.
Aimed at the right place: the split keeps heavy imports off the orchestrator, and moving the cache onto local_objects is what lets pipelines coexist.
2 correctness findings, 1 LLMisms. Both correctness ones come from the new release paths:
nodes/clear_pipeline_cache_node.py:24:drop_all()also releases parked latents.nodes/release_pipeline_node.py:49: releasing a base breaks the ControlNet pipeline built from it.
There was a problem hiding this comment.
2 of 3 findings fixed. 1 open:
release_pipeline_node.py:49— releasing a base pipeline tears down the ControlNet pipeline built from it.
Commit 205ac48 since the last review is clean against the rubric.
…v-exec The orchestrator imports every node module and constructs every node class to populate the editor, so anything it touches has to exist in the edit-time environment. This moves the heavy set (torch, diffusers, transformers, accelerate, numpy, PIL and friends) out of `pip_dependencies` into `pip_dependencies_exec`, which the engine installs additively into a worker's `.venv-exec`, and keeps the orchestrator's environment to what building the node classes actually needs. What that required: - Defer every heavy import. Annotation-only uses move into `TYPE_CHECKING` blocks, module-scope type aliases become lazily-evaluated `type` aliases, and runtime uses move into the function that needs them. Pipeline drivers resolve on lookup instead of at import. - Stop deriving node shape from diffusers. A node's `__init__` runs on the orchestrator, so anything previously read off a pipeline's signature is declared per pipeline type (`_component_slots`) and pinned by a test. - Hold built pipelines and latents in the worker's object store and pass them as references, so an unserializable value never has to cross the process boundary. - Validate where the value lives. Checks that inspect a held payload move to `validate_in_execution_environment`; `validate_before_node_run` keeps only what the orchestrator can answer. - Read config through requests rather than engine manager accessors, which refuse to answer during node execution in a worker. - Pin the CUDA index and pool PyPI alongside it, and raise the engine floor to 0.102.0. New gates, all wired into `make check`: `check/edit-time-imports` imports every node module and constructs every node class, then fails if a heavy package was reached; `check/worker-safe` fails on a manager access that is not a request; and the unit suite now runs from `check` so CI gates it.
`MediaGenConditioningRuntimeParameter.validate_before_node_run` is now `validate_in_execution_environment`, and these two still delegated to the old name, so validating either node raised `AttributeError` instead of returning errors. Rename the enclosing hook rather than just the call, matching the seven runtime-parameter classes already converted: the delegation belongs with the hook the conditioning class now exposes. Qwen Edit's own `image_references` check moves with it. Nothing here depends on the orchestrator -- the payload is URL artifacts and scalars and travels as data -- so running it where the node runs costs only the earlier failure point.
`ast.Constant.value` is any literal, so a `name=` or `type=` that is not a string satisfied the `isinstance(..., ast.Constant)` check and was returned where the signature promises `str` -- which pyright rejected, and which would have reached `_is_sendable`'s `.endswith` as a non-string.
Main removed this repo from the catalog without updating the test that asserts how it is keyed, so the lookup raised `KeyError`. Nothing in the library offers the repo any more. The surviving four match what the docstring already claims. This went unnoticed because main's `check` target does not run the tests; this branch adds that.
91d8bbf to
21a07d5
Compare
`huggingface_hub` is imported from four orchestrator-reachable paths but was declared nowhere. It arrived only because the host engine happens to depend on it, and on main because the edit-time `diffusers` pulled it in. Declared in both dependency sets. `pip_install_flags` goes back to main's `--preview --torch-backend=auto`. The pinned cu128 index was an elective change, and the goal is main's install in a new environment. `make check` now runs `test/exec` as well as `test/unit`. CI runs `make check` and nothing else, so the suite that checks this library's declarations against real diffusers was gating nothing. It needs no extra install: `check/types` already builds that venv.
A `ComponentArtifact` reaching the worker as a bare parameter value arrived as a dict and was silently dropped: `get_component_overrides` returned nothing for that slot and `get_override_config_kwargs` left it out of the config hash, so the pipeline built with the default component under a hash claiming the override. Each subclass now registers itself and `_cattrs_unstructure` stamps its name into the wire form, so `structure_component_artifact` can rebuild the concrete class. The converter cannot tell them apart on shape alone -- every field of every subclass has a default and two of them share `config_source` and `repo_ref` -- which is what made the polymorphic registration raise on the production path. An unknown tag raises rather than guessing. Reading an override's config no longer needs the pipeline class, so the builder's compatibility check runs on the orchestrator, where it refreshes on every override edit. `_find_denoiser_slot` reads the declared `_component_slots` rather than the pipeline's `__init__` signature, and a scheduler config comes from the cached `scheduler_config.json` rather than through a scheduler class's `load_config`. Building a pipeline also releases the one already resident, which is what `ModelCache` did: holding two models' worth of VRAM is not the same experience in a new environment. A derived build keeps its base, whose components it shares.
`ComponentSpec`, `InputParam` and `OutputParam` were read in `ModularPipelineBlocks` property bodies, `randn_tensor` and five `numpy` uses at module scope, and the LTX2 upsampler's classes came from a `TYPE_CHECKING` block but were used at runtime. The orchestrator imports every node module, so each of these was a crash there rather than a slow import. `_sync_num_inference_steps_for_distillation` no longer runs from `after_value_set`: it reads the sigma schedule, which needs diffusers. The driver derives the step count from that same schedule at build time, so a value left stale by an edit never reaches the pipeline. The three `build_if_needed` guards now say what the flag means, since False reads as a performance hint rather than "this process cannot import diffusers".
|
@griptapeops re-review |
There was a problem hiding this comment.
The one open thread from the last cycle (release_pipeline_node.py:49) was accepted as a risk and resolved; taking that. The branch was squashed and rebased, so I read the 6 commits after 6543e07.
1 new correctness finding, in 5acc16b:
pipeline_artifact.py:532: keeping the base doesn't protect it when the sibling derived pipeline gets released.
The builder node no longer puts _pipeline_cls in overrides-only build data, so _build_pipeline_from_overrides_only raised KeyError. Fall back to the params class's pipeline_cls(), as build_pipeline_from_build_data already does.
|
@griptapeops re-review |
# Conflicts: # modular_diffusion_nodes_library/nodes/noise_latent_node.py # modular_diffusion_nodes_library/parameters/generate_latent_parameters.py
There was a problem hiding this comment.
No open findings from earlier cycles. The pipeline_artifact.py:532 thread was resolved as pre-existing behavior; taking that. The ModelCache split-out matches main, and the _pipeline_cls fallback in 5d5b720 is correct.
1 advisory, with no line to anchor to because it now matches main:
latent_diffusion_pipeline_builder_node.py:91: the restoredstateoverride can't see the worker's cache. It runs on the orchestrator, wheremodel_cachestays empty (the DAG builder readsupstream_node.statethere), so a resolved builder always reports UNRESOLVED and runs again. That run hits the worker's cache, so the cost is a redundant builder run rather than a model reload. Fine to leave for #76.
This is the Diffusers migration to worker v2, not a
test vehicle: it runs the Text2Image template end to end in a worker, with no heavy dependency on the
orchestrator.
Stack: this PR keeps the library's
ModelCache, which now lives in the worker. Moving built pipelinesinto the engine's local object store, and the new Release Pipeline node, are in #76 on top of this one.
The shape of it
Two environments, and the library has to be correct in both.
pip_dependencies— 2 entries (griptape, huggingface-hub)pip_dependencies_exec— 36, including torch/diffusersEverything below follows from keeping those two honest.
Edit time: no heavy dependencies
The orchestrator imports every node module and constructs every node class to show a node in the editor.
Anything that reached from there had to be installed there, which cost a second multi-gigabyte install
and put every library's copy of a shared package on one
sys.path.TYPE_CHECKING; module-scope type aliases became PEP 695typestatements; runtime uses moved into the function that needs them.
@torch.no_grad()is evaluated while the class body runs, so 11 sites now useno_grad/inference_modefromutils/torch_utils.py, which enter the context at call time.get_component_slots()read a pipeline's__init__signature to decide which override ports to show —1730 heavy imports during node construction. Each pipeline type now declares
_component_slots, andtests/test_component_slots.pychecks all 19 against the real classes.produces_video,video_fps,supports_inpainting) are declared onDriverSpec,so asking "does this produce video" no longer imports a driver.
tests/test_driver_specs.pypins them.The dev environment enforces this too.
[project] dependenciesis the edit-time set alone; theexecution set is an
execextra.uv synctherefore gives a developer the same slim orchestrator a realinstall gets, and
scripts/sync_dependencies.pyrefuses to sync if anything heavy is declared edit-time.make test/execbuilds a separate.venv-test-execrather than fattening.venvand leaving it thatway. This mattered: a fat dev venv is what let orchestrator-side code build a pipeline locally and
silently eat 23GB.
huggingface-hubis declared, because the orchestrator imports it. Reading a model's cached config ishow the loader and builder nodes answer questions about a component without diffusers, and four
orchestrator-reachable paths do it. On
mainthe edit-timediffuserspulled it in; with that gone it wasarriving only because the host engine happens to depend on it, which is not a guarantee a library can rely
on.
ruffnow also enablesTC004— aTYPE_CHECKINGimport used at runtime raisesNameErrorwhileboth default ruff and pyright stay green, and this change moves enough imports into
TYPE_CHECKINGto makethat worth gating.
Correctness across the boundary
Five classes of bug, each found and then gated so it cannot come back.
1. A value that cannot be encoded is silently stringified. The engine writes event payloads with
json.dumps(default=str).DiffusionPipelineArtifactdefined__str__as its config hash, so itarrived from the worker as that hash string — no exception, a type error two nodes later. It now
travels via
_cattrs_unstructure/_cattrs_structure, the engine's seam for a class that knows its ownwire form, and
__str__is gone so a recurrence is unmistakable.2. A value that can travel must not be held.
output_imageandoutput_videowereserializable=False, so the editor received the cache's own envelope and rendered blank:Both are URL artifacts naming a file on the shared workspace. Removing the flag is the whole fix.
tests/test_parked_values.pychecks the rule statically and through the real egress path.3. Node code must not reach an engine manager. Those raise during worker execution: that process
holds its own copy of the state, so a local answer would be silently wrong.
scripts/check_worker_safe_managers.pyreads the guarded list out of the engine rather thanrestating it, and ignores construction-time reads, which a worker performs before the guarded scope
opens. It found a live failure the crash-driven approach had not reached yet —
connection_utilsreadconnections off
FlowManagerfromset_parameter_value, and hydration sets values withinitial_setupdefaulting to False.
4. A held value cannot be read outside its process. Not even to check whether it is set. Every such
check moved to
validate_in_execution_environment(engine #5650): the three pipeline-building dimensionchecks, the input-latent checks, ControlNet's
can_make_control_pipe_from_standard, and fourvalidate_before_node_runreads of a parked latent.5. A value that encodes but cannot be rebuilt is silently dropped. A
ComponentArtifactreaching theworker as a bare parameter value arrived as a dict:
get_component_overridesreturned nothing for thatslot and
get_override_config_kwargsleft it out of the config hash, so the pipeline built with thedefault component under a hash claiming the override. Each subclass now registers itself and
_cattrs_unstructurestamps its name into the wire form, so one function rebuilds the concrete class.Naming the class is what makes this work — the converter cannot tell the subclasses apart on shape alone,
since every field of every one of them has a default and two share
config_sourceandrepo_ref. Anunknown tag raises rather than guessing.
tests/test_pipeline_artifact_round_trip.pyandtest_parked_values.pydrive the engine's real encode path, and both fail if either the tag or theregistration is removed.
Manifest: the engine floor
metadata.engine_versionmoves from0.100.2to0.102.0, the first release carryingpip_dependencies_exec,BaseNode.execution_deviceand thevalidate_in_execution_environmenthookthis change depends on. Below that floor an engine ignores the execution set, builds no
.venv-exec,and the nodes then fail only when someone runs them.
pip_install_flagsstays onmain's--preview --torch-backend=auto. An earlier revision pinned a cu128--extra-index-url, on the reasoning thatautoresolves per installing machine and cannot be reconciledwith a hard
torch==2.7.0pin. That is a real concern, but it ismain's concern too, and this change ismeant to put the same dependencies in a new environment rather than to settle it.
Reviewer notes
CLAUDE.mdchanged twice, and both are policy rather than mechanics: the "do not use lazy imports"rule is now a deferral policy with its three mechanisms, and there is a new "Running in a worker"
section stating the two rules above with their gates. Worth reading — leaving the old rule would have
invited the next contributor to revert the whole change.
check/typesruns against the execution venv. Pyright cannot resolve torch or diffusers in theedit-time environment, and that absence is deliberate rather than a setup error.
make checknow runstest/exectoo. CI runsmake checkand nothing else, so the suite thatchecks this library's declarations (driver specs, component slots) against real diffusers was gating
nothing: an upstream signature change surfaced as wrong ports in the builder instead of a red build. It
needs no extra install, since
check/typesalready builds that venv.mainand the migration itself are one commit; the threeafter it are review fixes, each scoped to one claim. The last commit splits the object-cache migration
out to Hold Diffusers pipelines in the engine's local object store #76.
Verification
End to end in the editor: Text2Image runs in a worker and the decoded image appears.
none — in both environments
pip_dependenciesimports all 24 with zero heavy packagesBoth run in
make checkmain(Windows, py3.12, 2026-09-28): the execution resolve and the installed.venv-execmatchmain's install exactly, 159 packages each. Declaringhuggingface-hubdirectly doesnot move that count — it was already in the set, pulled in by diffusers and transformers
Known gaps
GriptapeNodes.X()written in this library. A callinto an engine helper that itself uses the facade is invisible to it, as is a direct manager import.
get_parameter_value("literal").misc/partial_denoise.pystates it assumes serial generation. Untouched, andparallel resolution exists.
OSManager.cleanup_directory_if_neededis reached only whenenable_directory_cleanupis on, whichdefaults to false. It is imported directly rather than through the facade, since it is static and
touches only the shared filesystem.
receiving process to have imported the module declaring that subclass. Both processes import all 24
node modules today, so this is latent rather than reachable; the error names the tag and the known set.
deps/syncis not checked in CI.pyproject.tomland the manifest can drift without failing abuild; they agree today.
parameters needs diffusers, so the unknown-key warning now appears after a run rather than on edit.