Skip to content

fix(extensions): validate model registration - #233

Open
DrHepa wants to merge 1 commit into
lightningpixel:devfrom
DrHepa:fix/extension-registration-validation
Open

fix(extensions): validate model registration#233
DrHepa wants to merge 1 commit into
lightningpixel:devfrom
DrHepa:fix/extension-registration-validation

Conversation

@DrHepa

@DrHepa DrHepa commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Fixes #232

Summary

  • make model-extension installation success depend on successful runtime registration
  • preserve actionable per-extension discovery errors instead of silently dropping failed generators
  • make Install, Repair, and local replacement crash-safe with rollback and durable quarantine
  • keep incomplete models repairable in the UI while excluding them from generation workflows

Key changes

  • isolates direct Python extension imports and reloads extension-owned modules without leaking extension roots through global sys.path
  • validates reload results strictly for model extensions while preserving the public reload response and process-extension behavior
  • records registration transactions outside extension trees, restores the previous version after failed updates, and prevents uninstall/restart resurrection
  • uses exact-destination, one-time validation capabilities for quarantined extensions
  • stops loaded extension runtimes before Repair mutates their environment
  • fails closed for unreadable or incompatible replacement manifests
  • preserves UI-managed Models data; no weights are downloaded, deleted, or moved

Validation

  • npm test — 47 Python and 110 Node tests passed in the configured Modly development environment
  • npm run lint
  • npm run build
  • Python compilation checks
  • git diff --check upstream/dev --
  • Node and web TypeScript diagnostics match the clean upstream/dev baseline

@lightningpixel

Copy link
Copy Markdown
Owner

Thanks for this, the design looks solid and the test coverage is impressive.

Quick question before I approve: did you actually run a generation with trellis2 on this branch ?

It's the one official extension with no setup.py (just build_vendor.py + a committed vendor/), so after this PR it stays on the direct/proxy path permanently, it can never get a venv.

Two things I couldn't verify by reading:

  1. Cancel during generation. call_in_context holds the manager-wide RLock for the whole call. POST /generate/cancel does hasattr(gen, "_proc") on the event loop, which goes through getattr → activate() → waits on that lock. Looks like the API would freeze for the entire generation.
  2. Vendored imports. _setup_vendor() inserts vendor/ into sys.path from load(), but activate() restores sys.path on exit. Any lazy import during generate() might then fail.

If you've tested it and both work, let me know and I'll drop it. Otherwise that's the scenario I'd want checked before merge.

@DrHepa

DrHepa commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — I tested the current functional Trellis2 GGUF extension at commit d707382 in a completely isolated environment on PR #233.
This repository does have a setup.py and creates an extension venv. Both trellis2/generate and trellis2/refine therefore register as ExtensionProcess; neither uses the direct proxy/RLock path.

I exercised the real FastAPI reload and cancellation routes. Reload registered both nodes with no errors, and cancellation returned HTTP 200 in 0.86 ms, reached the cancelled state, and produced zero legacy proxy/context activations.

Lazy imports also occur inside the extension subprocess using its own venv, so restoring the manager’s sys.path does not affect this GGUF extension.

I want to be precise about the limit: I could not complete a real GLB generation on this ARM64 GB10 machine. Trellis2’s setup exits 0, but the required cumesh and flex_gemm wheels are unavailable for ARM64. Registration and the worker ready handshake succeed, but generation readiness does not. That appears to be an existing Trellis2 GGUF setup/platform issue rather than a regression from this PR.
Based on the functional GGUF extension path, I could not reproduce either of the two PR concerns.

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