Merge pull request #18 from m-cahill/m05-override-isolation

M05: Override isolation / temporary opts seam
2026-04-20 11:51:09 -07:00 · 2026-03-09 15:27:20 -07:00 · 2026-03-09 15:27:20 -07:00 · 6fd992ae27
commit 6fd992ae27
parent 9952611b02 cbf1dbc59b
6 changed files with 591 additions and 62 deletions
--- a/docs/milestones/M05/M05_plan.md
+++ b/docs/milestones/M05/M05_plan.md
@ -1,62 +1,355 @@
 # M05 Plan — Override Isolation / Temporary Opts Seam

-**Milestone:** M05  
-**Title:** Override isolation / temporary opts seam  
-**Branch:** `m05-override-isolation`  
-**Status:** Planned  
-**Depends on:** M04 (complete)
+**Project:** Serena
+**Phase:** Phase II — Runtime Seam Preparation
+**Milestone:** M05
+**Title:** Override Isolation / Temporary Opts Seam
+**Branch:** `m05-override-isolation`
+**Posture:** Behavior-Preserving Refactor
+**Target:** Introduce an isolated mechanism for temporary option overrides during generation.

 ---

-## 1. Intent / Target
+# 1. Intent / Target

-Introduce the first architectural seam for Phase II: isolate override_settings application and restore from `process_images` into a reusable context manager or helper. This prepares for opts snapshot threading (M07–M08) and reduces direct mutation of global `shared.opts` during a run.
+Introduce a **temporary options override seam** that prevents direct mutation of `shared.opts` during generation runs.

-No runtime behavior changes. Override application and restore logic must remain identical.
+Currently, `process_images()` temporarily mutates global options when applying `override_settings`, then restores them afterward.
+
+This milestone introduces a **context-managed override mechanism** that:
+
+* isolates temporary option changes
+* preserves runtime behavior
+* prepares the runtime pipeline for future **opts snapshot injection**
+
+This is the **first runtime seam milestone** in Phase II.

 ---

-## 2. Scope Boundaries
+# 2. Problem Being Solved
+
+The current implementation inside `modules/processing.py` roughly resembles:
+
+```python
+for k, v in p.override_settings.items():
+    opts.set(k, v)
+
+process_images_inner(p)
+
+restore_settings()
+```
+
+Issues:
+
+* mutates **global state**
+* creates potential nondeterminism
+* makes testing harder
+* couples generation pipeline to `shared.opts`
+
+This milestone **does not change behavior**, but introduces a **structured override mechanism**.
+
+---
+
+# 3. Scope Boundaries

 ### In scope

- Extract override apply/restore block in `process_images` into a context manager or helper
- Introduce `temporary_opts(override_settings)` or equivalent seam
- Preserve exact semantics: apply overrides before inner processing, restore in `finally`
- Add unit test for the seam (mock opts, verify apply/restore)
+* Introduce a **temporary options context manager**
+* Replace inline override logic in `process_images()`
+* Ensure restoration logic is deterministic
+* Preserve identical runtime semantics

 ### Explicitly out of scope

- Opts snapshot (immutable view) — M07
- Passing opts into `process_images_inner` — M08
- Changing override_settings semantics
- API or UI changes
+* Changing how `opts` values are accessed elsewhere
+* Introducing `opts_snapshot` (M07)
+* Changing processing pipeline structure
+* Any modification to API/UI behavior
+* Performance changes

 ---

-## 3. Current Behavior (Evidence)
+# 4. Invariants (Must Not Change)

-From `processing.py:823-857`:
+The following runtime surfaces **must remain identical**:

- Override settings are applied to `shared.opts` via `opts.set(key, value)` before `process_images_inner`
- In `finally`, if `override_settings_restore_afterwards`, opts are restored
- This block is the target for extraction
+| Surface              | Verification                  |
+| -------------------- | ----------------------------- |
+| Image outputs        | Smoke tests                   |
+| API response schemas | API tests                     |
+| CLI behavior         | Smoke tests                   |
+| Extension behavior   | Extension loading smoke       |
+| Generation semantics | txt2img / img2img smoke tests |
+
+These invariants are part of the Serena invariant registry.

 ---

-## 4. Implementation Approach
+# 5. Verification Plan

-1. Create helper or context manager (e.g. `modules/opts_override.py` or in `processing.py`)
-2. Replace inline override block in `process_images` with call to the helper
-3. Add minimal unit test that verifies apply/restore behavior
-4. Ensure no behavior change; smoke and quality tests pass
+### CI gates expected to remain green
+
+* Smoke tests
+* Quality tests
+* Coverage ≥ 40%
+* verify_pinned_deps
+* pip-audit (informational)
+
+### Evidence artifacts
+
+CI should still produce:
+
+```
+coverage.xml
+ci_environment.txt
+```
+
+as introduced in M04.
+
+### Behavioral verification
+
+* Compare outputs from smoke generation tests
+* Ensure override settings behave identically

 ---

-## 5. Definition of Done
+# 6. Implementation Steps

- [ ] Override apply/restore extracted to reusable seam
- [ ] `process_images` uses the seam; logic unchanged
- [ ] Unit test for seam
- [ ] Smoke and Quality CI green
- [ ] Milestone docs and ledger update
+## Step 1 — Add temporary override context manager
+
+Create helper:
+
+```
+modules/runtime_utils.py
+```
+
+(or similar location appropriate to project structure)
+
+Add:
+
+```python
+from contextlib import contextmanager
+from modules import shared
+
+
+@contextmanager
+def temporary_opts(overrides: dict):
+    if not overrides:
+        yield
+        return
+
+    original = {}
+
+    try:
+        for key, value in overrides.items():
+            if hasattr(shared.opts, key):
+                original[key] = getattr(shared.opts, key)
+                shared.opts.set(key, value)
+
+        yield
+
+    finally:
+        for key, value in original.items():
+            shared.opts.set(key, value)
+```
+
+Purpose:
+
+* isolate override logic
+* centralize restore semantics
+* enable later replacement with snapshot model
+
+---
+
+## Step 2 — Replace override block in `process_images()`
+
+Locate override logic inside:
+
+```
+modules/processing.py
+```
+
+Replace pattern similar to:
+
+```python
+for k, v in p.override_settings.items():
+    opts.set(k, v)
+
+process_images_inner(p)
+
+restore_settings()
+```
+
+with:
+
+```python
+with temporary_opts(p.override_settings):
+    process_images_inner(p)
+```
+
+---
+
+## Step 3 — Remove redundant restore logic
+
+Remove manual restore code now handled by context manager.
+
+Ensure behavior remains identical.
+
+---
+
+## Step 4 — Minimal unit test
+
+Add small test under:
+
+```
+test/quality/test_opts_override.py
+```
+
+Example:
+
+```python
+def test_temporary_opts_restores_value():
+    from modules import shared
+    from modules.runtime_utils import temporary_opts
+
+    original = shared.opts.some_option
+
+    with temporary_opts({"some_option": "test_value"}):
+        assert shared.opts.some_option == "test_value"
+
+    assert shared.opts.some_option == original
+```
+
+Purpose:
+
+* verify restoration behavior
+* protect seam for future milestones
+
+---
+
+## Step 5 — Ensure no behavior drift
+
+Run:
+
+```
+pytest
+```
+
+Verify:
+
+* generation tests unchanged
+* API tests unchanged
+* coverage still ≥ 40%
+
+---
+
+# 7. Risk & Rollback Plan
+
+### Risk
+
+Low.
+
+Changes are isolated to override application.
+
+### Potential issue
+
+If extensions rely on exact ordering of override logic.
+
+### Rollback
+
+Revert the commit introducing:
+
+```
+temporary_opts
+```
+
+and restore original override block.
+
+Because the change is localized, rollback is trivial.
+
+---
+
+# 8. Deliverables
+
+### Code
+
+New helper:
+
+```
+modules/runtime_utils.py
+```
+
+Modified:
+
+```
+modules/processing.py
+```
+
+New test:
+
+```
+test/quality/test_opts_override.py
+```
+
+---
+
+### Documentation
+
+Update milestone artifacts:
+
+```
+docs/milestones/M05/M05_summary.md
+docs/milestones/M05/M05_audit.md
+```
+
+Update ledger:
+
+```
+docs/serena.md
+```
+
+---
+
+# 9. Acceptance Criteria
+
+M05 is complete when:
+
+* CI passes
+* Coverage ≥ 40%
+* Override logic replaced with context manager
+* Runtime behavior unchanged
+* Milestone documentation completed
+* Audit score remains **5.0**
+
+---
+
+# 10. Expected Architectural Impact
+
+Before:
+
+```
+process_images
+ └─ mutate shared.opts
+```
+
+After:
+
+```
+process_images
+ └─ temporary_opts
+     └─ process_images_inner
+```
+
+This creates the **first runtime seam** required for Phase II.
+
+---
+
+# 11. Next Milestone
+
+```
+M06 — Prompt / Seed Preparation Extraction
+```
+
+Goal:
+
+Extract prompt + seed preparation logic from `process_images_inner()`.
--- a/docs/milestones/M05/M05_run1.md
+++ b/docs/milestones/M05/M05_run1.md
@ -0,0 +1,165 @@
+# M05 CI Run 1 — Override Isolation / Temporary Opts Seam
+
+**Date:** 2026-03-09  
+**Branch:** m05-override-isolation  
+**PR:** #18  
+**Trigger:** pull_request (PR to main)
+
+---
+
+## 1. Workflow Identity
+
+| Workflow | Run ID | Trigger | Branch | Commit | Status |
+|----------|--------|---------|--------|--------|--------|
+| Smoke Tests | 22876253113 | pull_request | m05-override-isolation | 5fe82459 | ✓ success |
+| Linter | 22876253091 | pull_request | m05-override-isolation | 5fe82459 | ✓ success |
+
+**Quality Tests:** Not yet run (triggered on push to main; will run post-merge).
+
+---
+
+## 2. Change Context
+
+| Item | Value |
+|------|-------|
+| Milestone | M05 — Override isolation / temporary opts seam |
+| Phase | Phase II — Runtime Seam Preparation |
+| Posture | Behavior-preserving |
+| Refactor target | `modules/processing.py` (override apply/restore), new `modules/runtime_utils.py` |
+| Run type | Corrective (first CI verification of M05 implementation) |
+
+---
+
+## 3. Step 1 — Workflow Inventory
+
+### Smoke Tests (22876253113)
+
+| Job / Step | Required? | Purpose | Pass/Fail |
+|------------|-----------|---------|-----------|
+| Verify repository | Yes | Guardrail: m-cahill/serena only | ✓ |
+| Verify base branch | Yes | Guardrail: PR targets main | ✓ |
+| Checkout Code | Yes | Fetch PR branch | ✓ |
+| Set up Python 3.10 | Yes | Runtime | ✓ |
+| Cache models | Yes | Deterministic model path | ✓ |
+| Install test dependencies | Yes | pytest, coverage | ✓ |
+| Install runtime dependencies | Yes | torch, CLIP, open_clip, requirements_versions | ✓ |
+| Create stub repositories | Yes | CI fake inference support | ✓ |
+| Setup environment | Yes | launch.py --exit | ✓ |
+| Smoke startup | Yes | Verify server can start | ✓ |
+| Start test server | Yes | Live server for API tests | ✓ |
+| **Run smoke tests** | **Yes** | **pytest test/smoke** | **✓** |
+| Kill test server | Yes | Cleanup | ✓ |
+| Upload main app output | No (always) | Artifact for debugging | ✓ |
+
+**Duration:** 2m48s (Run smoke tests ~45s)
+
+### Linter (22876253091)
+
+| Job | Required? | Purpose | Pass/Fail |
+|-----|-----------|---------|-----------|
+| ruff | Yes | Python lint | ✓ |
+| eslint | Yes | JS lint | ✓ |
+
+---
+
+## 4. Step 2 — Refactor Signal Integrity
+
+### A) Tests
+
+- **Tier:** Smoke only (test/smoke)
+- **Coverage of refactor target:** Smoke tests exercise txt2img and img2img API endpoints, which call `process_images()` → `temporary_opts` → `process_images_inner`. The override seam is on the critical path.
+- **Failures:** None
+- **Golden/snapshot:** Smoke tests use CI fake inference (deterministic 1×1 PNG); no golden image comparison. API contract (response schema) is exercised.
+- **Missing:** `test_opts_override.py` (quality tier) did not run; it will run on push to main.
+
+### B) Coverage
+
+- Smoke run does not enforce a coverage gate (Quality Tests do, on push to main).
+- Coverage gate: ≥40% (M04 baseline).
+- Post-merge Quality run will report coverage and run `test_opts_override.py`.
+
+### C) Static / Policy Gates
+
+- Ruff and eslint passed on new and modified files.
+- No import boundary breaks, circular deps, or layering violations observed.
+
+### D) Security / Supply Chain
+
+- Not run in Smoke (Quality runs pip-audit).
+
+### E) Performance
+
+- No benchmarks. Smoke tests complete in expected time (~45s for pytest).
+
+---
+
+## 5. Step 3 — Delta Analysis
+
+### Change inventory
+
+| File | Change |
+|------|--------|
+| modules/runtime_utils.py | New (temporary_opts context manager) |
+| modules/processing.py | Modified (use temporary_opts, preserve model/VAE reload, token merging) |
+| test/quality/test_opts_override.py | New (unit tests for seam) |
+| docs/milestones/M05/* | Plan, toolcalls |
+
+**Public surfaces:** None. API endpoints, CLI, schemas unchanged.
+
+### Expected vs observed
+
+- **Expected:** Override logic relocated to context manager; behavior identical.
+- **Observed:** Smoke tests pass; txt2img/img2img path exercises the new seam without regression.
+
+### Refactor-specific drift
+
+- **Signal drift:** None. All required checks ran and passed.
+- **Coupling revealed:** None.
+- **Hidden dependencies:** None observed.
+
+---
+
+## 6. Step 4 — Failure Analysis
+
+No failures. All jobs passed.
+
+---
+
+## 7. Step 5 — Invariants & Guardrails Check
+
+| Invariant | Status |
+|-----------|--------|
+| Required checks enforced | ✓ Smoke, Linter both required and passed |
+| No scope creep | ✓ Only override seam; no feature work |
+| Public surfaces compatible | ✓ API/CLI/schemas unchanged |
+| Schema/contract outputs valid | ✓ Smoke exercises API responses |
+| Determinism preserved | ✓ CI fake inference; no golden drift |
+| No green-but-misleading path | ✓ No skips, continues, or muted gates |
+
+---
+
+## 8. Step 6 — Verdict
+
+**Verdict:** Smoke Tests and Linter both passed. The refactor target (`process_images` → `temporary_opts`) is on the critical path exercised by smoke tests (txt2img, img2img). No behavioral drift observed. Quality Tests (including `test_opts_override.py` and coverage) will run post-merge on push to main.
+
+**Recommended outcome:** ✅ **Merge approved**
+
+---
+
+## 9. Step 7 — Next Actions
+
+| Action | Owner | Scope | Milestone |
+|--------|-------|-------|-----------|
+| Merge PR #18 | Human | m05-override-isolation → main | M05 |
+| Verify Quality run post-merge | Human/Cursor | Run ID, coverage ≥40%, test_opts_override pass | M05 |
+| Generate M05_summary, M05_audit, ledger update | Cursor | After Quality green | M05 closeout |
+
+---
+
+## 10. Summary Table
+
+| Check | Run ID | Result |
+|-------|--------|--------|
+| Smoke Tests | 22876253113 | ✓ success |
+| Linter | 22876253091 | ✓ success |
+| Quality Tests | — | Pending (push to main) |
--- a/docs/milestones/M05/M05_toolcalls.md
+++ b/docs/milestones/M05/M05_toolcalls.md
@ -7,4 +7,9 @@

 | Timestamp | Tool | Purpose | Files/Target | Status |
 |-----------|------|---------|--------------|--------|
-| (seeded) | — | M05 plan and toolcalls scaffold | docs/milestones/M05/ | done |
+| 2026-03-09 | write | Create runtime_utils.py | modules/runtime_utils.py | done |
+| 2026-03-09 | search_replace | Refactor process_images to use temporary_opts | modules/processing.py | done |
+| 2026-03-09 | write | Add test_opts_override.py | test/quality/test_opts_override.py | done |
+| 2026-03-09 | ruff | Lint runtime_utils, test_opts_override | modules/runtime_utils.py, test/quality/test_opts_override.py | pass |
+| 2026-03-09 | git | Create branch, commit, push | m05-override-isolation, 5fe82459 | done |
+| 2026-03-09 | write | Generate M05_run1.md | docs/milestones/M05/M05_run1.md | done |
--- a/modules/processing.py
+++ b/modules/processing.py
@ -26,6 +26,7 @@ import modules.paths as paths
 import modules.face_restoration
 import modules.images as images
 import modules.styles
+import modules.runtime_utils as runtime_utils
 import modules.sd_models as sd_models
 import modules.sd_vae as sd_vae
 from ldm.data.util import AddMiDaS
@ -820,42 +821,32 @@ def process_images(p: StableDiffusionProcessing) -> Processed:
    if p.scripts is not None:
        p.scripts.before_process(p)

-    stored_opts = {k: opts.data[k] if k in opts.data else opts.get_default(k) for k in p.override_settings.keys() if k in opts.data}
+    # if no checkpoint override or the override checkpoint can't be found, remove override entry and load opts checkpoint
+    # and if after running refiner, the refiner model is not unloaded - webui swaps back to main model here, if model over is present it will be reloaded afterwards
+    if sd_models.checkpoint_aliases.get(p.override_settings.get('sd_model_checkpoint')) is None:
+        p.override_settings.pop('sd_model_checkpoint', None)
+        sd_models.reload_model_weights()

    try:
-        # if no checkpoint override or the override checkpoint can't be found, remove override entry and load opts checkpoint
-        # and if after running refiner, the refiner model is not unloaded - webui swaps back to main model here, if model over is present it will be reloaded afterwards
-        if sd_models.checkpoint_aliases.get(p.override_settings.get('sd_model_checkpoint')) is None:
-            p.override_settings.pop('sd_model_checkpoint', None)
-            sd_models.reload_model_weights()
+        with runtime_utils.temporary_opts(p.override_settings, restore_afterwards=p.override_settings_restore_afterwards):
+            for k in p.override_settings:
+                if k == 'sd_model_checkpoint':
+                    sd_models.reload_model_weights()
+                if k == 'sd_vae':
+                    sd_vae.reload_vae_weights()

-        for k, v in p.override_settings.items():
-            opts.set(k, v, is_api=True, run_callbacks=False)
+            sd_models.apply_token_merging(p.sd_model, p.get_token_merging_ratio())

-            if k == 'sd_model_checkpoint':
-                sd_models.reload_model_weights()
-
-            if k == 'sd_vae':
-                sd_vae.reload_vae_weights()
-
-        sd_models.apply_token_merging(p.sd_model, p.get_token_merging_ratio())
-
-        # backwards compatibility, fix sampler and scheduler if invalid
-        sd_samplers.fix_p_invalid_sampler_and_scheduler(p)
-
-        with profiling.Profiler():
-            res = process_images_inner(p)
+            # backwards compatibility, fix sampler and scheduler if invalid
+            sd_samplers.fix_p_invalid_sampler_and_scheduler(p)

+            with profiling.Profiler():
+                res = process_images_inner(p)
    finally:
        sd_models.apply_token_merging(p.sd_model, 0)

-        # restore opts to original state
-        if p.override_settings_restore_afterwards:
-            for k, v in stored_opts.items():
-                setattr(opts, k, v)
-
-                if k == 'sd_vae':
-                    sd_vae.reload_vae_weights()
+        if p.override_settings_restore_afterwards and 'sd_vae' in p.override_settings:
+            sd_vae.reload_vae_weights()

    return res

--- a/modules/runtime_utils.py
+++ b/modules/runtime_utils.py
@ -0,0 +1,41 @@
+"""Runtime utilities for the generation pipeline.
+
+M05: Temporary opts override seam — isolates option mutation during generation.
+"""
+from contextlib import contextmanager
+
+from modules import shared
+
+
+@contextmanager
+def temporary_opts(overrides: dict, restore_afterwards: bool = True):
+    """Context manager for temporary option overrides during generation.
+
+    Applies overrides via opts.set(..., is_api=True, run_callbacks=False).
+    Restores original values on exit via setattr (no callbacks).
+    Only mutates keys present in opts.data.
+
+    Args:
+        overrides: Dict of option key -> value to apply.
+        restore_afterwards: If True, restore original values on exit.
+    """
+    opts = shared.opts
+    if not overrides:
+        yield
+        return
+
+    original = {
+        k: opts.data[k] if k in opts.data else opts.get_default(k)
+        for k in overrides.keys()
+        if k in opts.data
+    }
+
+    try:
+        for k, v in overrides.items():
+            if k in opts.data:
+                opts.set(k, v, is_api=True, run_callbacks=False)
+        yield
+    finally:
+        if restore_afterwards:
+            for k, v in original.items():
+                setattr(opts, k, v)
--- a/test/quality/test_opts_override.py
+++ b/test/quality/test_opts_override.py
@ -0,0 +1,34 @@
+"""Unit tests for temporary_opts context manager (M05 override isolation seam)."""
+from modules import shared
+from modules.runtime_utils import temporary_opts
+
+
+def test_temporary_opts_restores_value(initialize):
+    """temporary_opts restores samples_save to original value on exit."""
+    original = shared.opts.samples_save
+
+    with temporary_opts({"samples_save": False}):
+        assert shared.opts.samples_save is False
+
+    assert shared.opts.samples_save == original
+
+
+def test_temporary_opts_restore_afterwards_false(initialize):
+    """When restore_afterwards=False, value is not restored."""
+    original = shared.opts.samples_save
+    try:
+        with temporary_opts({"samples_save": False}, restore_afterwards=False):
+            assert shared.opts.samples_save is False
+        assert shared.opts.samples_save is False
+    finally:
+        shared.opts.samples_save = original
+
+
+def test_temporary_opts_empty_overrides(initialize):
+    """Empty overrides yields without mutation."""
+    original = shared.opts.samples_save
+
+    with temporary_opts({}):
+        pass
+
+    assert shared.opts.samples_save == original