Skip to content

feat(workflows): support scene model artifacts - #357

Merged
lightningpixel merged 4 commits into
lightningpixel:devfrom
DrHepa:feat/354-scene-artifacts
Oct 2, 2026
Merged

lightningpixel merged 4 commits into
lightningpixel:devfrom
DrHepa:feat/354-scene-artifacts

Conversation

@DrHepa

@DrHepa DrHepa commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Closes #354

Summary

  • add scene as a first-class input and output for model extension nodes
  • add a Load Scene workflow source and scene artifact registration
  • add a generic typed /generate/from-artifact route while preserving /generate/from-image
  • validate scene directories at submission and again inside the runner
  • bind queued generation and cancellation to the exact requested model across both generation APIs

Scope

This PR supports the reviewable model-node shapes needed by scene workflows:

  • scene -> mesh
  • scene -> scene
  • multi-image -> scene

Scene inputs and outputs remain restricted to model nodes with the supported single-scene shape. This PR does not implement capture or video artifact execution, but it no longer rejects unrelated third-party custom I/O declarations. Shared weight groups remain independent in #348.

Shared-weight integration

The Pixal3D consumer requires both this scene contract and shared-weight PR #348. An isolated merge of #348 + #357 + video PR #358 found three overlapping validation conflicts in generator_registry.py, extension-install-utils.ts, and ipc-handlers.ts. The combined resolution preserves scene/video artifact validation, ordered node inputs, and node-specific shared-weight projection.

Either PR can merge first; the later branch must preserve those combined behaviors during rebase.

Security and compatibility

  • rejects traversal, absolute and encoded paths, symlink/reparse escapes, malformed manifests, missing referenced files, and oversized scenes
  • enforces the 1 MiB scene-manifest limit before Electron reads or transfers the file, with Python defense in depth
  • reserves and strips transport parameters so callers cannot forge the artifact kind or path
  • serializes the complete pinned model lifecycle on a dedicated worker, avoiding dual residency, cancellation misrouting, and default-executor starvation
  • keeps existing image-model extensions and /generate/from-image behavior compatible
  • preserves third-party custom I/O declarations while validating supported scene shapes specifically
  • registers scene outputs as workflow artifacts rather than attempting to render them as meshes

Maintainer review fixes

  • rebased onto current dev and retained the fix(workflows): multi-input nodes drop their audio slot #359 slot-input implementation
  • clear validated scene state whenever the path changes and ignore stale async validation results
  • make Windows symlink coverage skip only when the OS denies symlink creation
  • make the default typed-artifact generator contract raise NotImplementedError
  • remove the redundant model_status fallback and give scenes a distinct pink color

Validation

  • Python: 154 tests passed
  • Node: 29 tests passed
  • Electron/Vite production build: passed
  • TypeScript: zero new diagnostics; the same 9 Node and 21 Web diagnostics reproduce on upstream/dev
  • git diff --check: passed
  • fresh adversarial review: no blocking findings

No real model inference, literal UI run, or Windows execution was performed in this environment.

@lightningpixel lightningpixel left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough work on this, especially the path validation on the Python side. A few things need to change before this can be merged:

  1. VRAM leak. /generate/from-artifact never calls switch_model(), and _run_generation now uses get_ready_generator(), which loads the model without unloading the active one. After a scene run, both models stay loaded. Switching back to the previous model is then a no-op, so the scene model is never unloaded. Please switch models the same way /from-image does.

  2. Load Scene can run a stale scene. Editing the path field keeps the previously validated manifestPath, and preflight and the runner only read manifestPath. So the workflow runs on the old scene while the node shows the new path. Please clear manifestPath whenever the path changes.

  3. Rebase on dev. It conflicts with #359, which moved the multi-input slot loop into slotInputs.ts. The scene branch in that loop can't be reached anyway, since scene inside inputs is rejected, so it can simply be dropped.

  4. Windows test failure. test_rejects_symlinks_missing_assets_and_oversized_manifest fails on Windows without symlink privileges (WinError 1314). Please skip it on OSError, or use a junction on Windows, which is also the more realistic case there.

  5. Stricter IO type validation. Install, the Electron listing and the registry now reject any input/output outside image|text|mesh|audio|scene. That's a breaking change for existing third-party extensions and should be called out. The allowed list is also hardcoded in four places, which is why this conflicts with #358. Could it live in one shared place?

Smaller points:

  • BaseGenerator.generate_artifact falls back to generate(artifact_path, ...), which passes a Path where bytes are expected. Raising NotImplementedError would be cleaner.
  • The getattr(generator_registry, "model_status", None) fallback isn't needed, since model_status is added in this same PR.
  • scene uses the same emerald color as audio for badges and handles.

@DrHepa
DrHepa force-pushed the feat/354-scene-artifacts branch from f46086d to 10cdc15 Compare October 1, 2026 20:53
@DrHepa

DrHepa commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the review in 10cdc15c and rebased the branch onto current dev.

  • model switching/loading and cancellation are now bound to the exact queued job across both generation routes; the complete pinned lifecycle is serialized on a dedicated worker
  • Load Scene clears validated state on path edits and ignores stale async validation results
  • the fix(workflows): multi-input nodes drop their audio slot #359 slot-input implementation is retained and the unreachable scene multi-input branch is removed
  • the Windows symlink test skips when the OS denies symlink creation
  • custom third-party I/O declarations remain accepted; only supported scene shapes are constrained
  • generate_artifact() now raises NotImplementedError, the redundant status fallback is gone, and scene UI uses a distinct pink color
  • scene manifests over 1 MiB are rejected by Electron before read/transfer, with Python validation retained

Validation: 154 Python tests + 29 Node tests passed, production build passed, git diff --check passed, and the PR adds no TypeScript diagnostics beyond the current dev baseline. A fresh adversarial review found no blocking issues.

I did not claim real model inference, literal UI execution, or Windows runtime validation from this environment.

- Read-only registry calls (model/active/all status, params_schema) no
  longer take the lifecycle lock, which load() holds for its whole duration
  (first-run downloads included); /model/status, /model/all and
  /model/params stay responsive while a model loads
- unload_all, reload and update_paths now take the lock so the lifecycle is
  actually serialized; their routes run them off the event loop
- Show "Waiting for the previous generation..." while a job is queued behind
  another on the single pinned worker
- Share RESERVED_ARTIFACT_PARAMS between the generation router and runner,
  and the scene-shape rule between preflight and the workflow runner
- Cover Windows junction escapes with a real test, and add a regression test
  for status reads during an in-progress load
# Conflicts:
#	src/areas/workflows/workflowRunStore.ts
@lightningpixel
lightningpixel merged commit 39cc58a into lightningpixel:dev Oct 2, 2026
lightningpixel added a commit to DrHepa/modly that referenced this pull request Oct 2, 2026
Resolve conflicts with the scene artifacts PR (lightningpixel#357):
- generator_registry: keep both the weight-group and scene-shape manifest
  validation; move the shared-weight readiness check into
  get_ready_generator; recompute shared_model_dirs inside the lifecycle lock
  in update_paths; unload_all unloads every model before reporting the ones
  that stayed loaded
- extension-install-utils / ipc-handlers: apply the scene-shape check, then
  the weight-group validation
- test_generator_registry: keep the tests from both sides
lightningpixel added a commit that referenced this pull request Oct 2, 2026
Resolve conflicts with the scene artifacts (#357) and shared weight groups
(#348) work, and address the review:

Conflict resolution
- Keep weight_groups and weight_variants side by side in the manifest
  validation (Python registry, model-sources, download plan, install utils,
  IPC listing), the preload API, the shared types and the Models UI
- Rebuild model-sources.ts and the interleaved tests from dev, re-adding the
  variant code and tests unchanged
- Download: keep dev's leased, target-aware flow and run the variant passes
  (shared files first, then the variant) in the legacy branch
- ModelsPage: keep dev's install queue and shared groups, pass the variant id
  through, and use installedNodeIds everywhere

Review fixes
- Check the job's model for a missing weight variant
  (assert_weight_variant_installed(params, model_id)): pinned jobs only switch
  to their model once they run, so the active model is not a stand-in
- deleteWeightVariant goes through the weight lease with a confirmed unload,
  and lists the variant files only once the node root is reserved
- Picking a missing variant no longer navigates away from the graph or the
  Generate panel; an "install it" link is shown instead
- Reject weight_variants combined with weight_groups with an explicit message
- Disable other variant installs while one variant of the node downloads
- README: undeclared repository variants are fetched by the shared pass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants