abinazebinoy commited on
Commit
cfffa10
Β·
1 Parent(s): 530738b

test(dire): regression test for scheduler isolation + doc update

Browse files

test_scheduler_is_not_shared_across_instances: two DIREDetector
instances that both hit the cache must get two distinct scheduler
objects, not references to the same one. Verified as a real
regression test, not just plausible -- reverted the fix and confirmed
this test fails with exactly the shared-object assertion error,
restored it and confirmed it passes.

Updates PROFILING_F17.md's scheduler-sharing section to reflect the
fix -- marked resolved, with a note that this removes the
thread-safety blocker on ever parallelizing DIRE/CLIP/own-embedding
but doesn't by itself answer whether it's worth doing (still needs a
real DIRE timing number, unchanged from before this fix).

PROFILING_F17.md CHANGED
@@ -63,17 +63,36 @@ for _ in range(3):
63
  print(f"DIRE mean: {statistics.mean(times):.2f}s (min {min(times):.2f}s, max {max(times):.2f}s)")
64
  ```
65
 
66
- ## The scheduler-sharing risk (why DIRE/CLIP/own-embedding are deliberately NOT parallelized yet)
67
 
68
- DIRE's `DDIMScheduler` is loaded once and cached process-wide (`ModelCache`, F-7) β€”
69
- `cached_model['scheduler']` β€” and every `DIREDetector` instance shares that *same object*. Both
70
  `_add_noise()` and the denoising loop call mutating methods on it (`scheduler.add_noise()`,
71
- `scheduler.set_timesteps()`, `scheduler.step()`). `denoise_steps` is currently hardcoded to `20`,
72
- so a `set_timesteps(20)` race between two concurrent DIRE calls likely produces the same values
73
- either way β€” probably not silently wrong today. But that's incidental, not a guarantee, and this
74
- is exactly the risk the original F-17 planning flagged as "a plausible interaction risk nobody has
75
- tested yet." Resolve this (explicit thread-safety test, or a per-instance scheduler clone instead
76
- of sharing the cached one) before enabling concurrent DIRE calls.
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
77
 
78
  ## What was actually shipped this round (partial F-17)
79
 
 
63
  print(f"DIRE mean: {statistics.mean(times):.2f}s (min {min(times):.2f}s, max {max(times):.2f}s)")
64
  ```
65
 
66
+ ## The scheduler-sharing risk β€” RESOLVED
67
 
68
+ DIRE's `DDIMScheduler` was loaded once and cached process-wide (`ModelCache`, F-7) β€”
69
+ `cached_model['scheduler']` β€” with every `DIREDetector` instance sharing that *same object*. Both
70
  `_add_noise()` and the denoising loop call mutating methods on it (`scheduler.add_noise()`,
71
+ `scheduler.set_timesteps()`, `scheduler.step()`). `denoise_steps` is hardcoded to `20`, so a
72
+ `set_timesteps(20)` race between two concurrent DIRE calls likely produced the same values either
73
+ way β€” probably not silently wrong in practice. But that was incidental, not a guarantee, and it was
74
+ exactly the risk the original F-17 planning flagged as "a plausible interaction risk nobody has
75
+ tested yet."
76
+
77
+ **Fixed** (follow-up commit, after this document was first written): `DIREDetector._load_model()`
78
+ now clones a fresh scheduler from the cached one's config on every cache hit
79
+ (`type(cached_scheduler).from_config(cached_scheduler.config)`) instead of sharing the cached
80
+ object directly. `from_config()` does no disk/network I/O β€” just Python object construction from a
81
+ config dict β€” so this is negligible overhead per request. `self.pipe` (the actual UNet/VAE weights)
82
+ stays shared: frozen, eval-mode forward passes don't mutate shared state, so sharing that part
83
+ remains safe. Verified directly: two `DIREDetector` instances that both hit the cache now get two
84
+ distinct scheduler objects (`first.scheduler is not second.scheduler`); confirmed this is a real
85
+ fix, not just plausible, by reverting it and watching the new regression test fail with exactly the
86
+ shared-object assertion error, then pass again once restored.
87
+
88
+ **This does not by itself mean DIRE should join the parallel signal pool.** The remaining open
89
+ question is the one this document's "DIRE / CLIP / own-embedding β€” structural estimate, not
90
+ measured" section above already raised: there's still no real DIRE timing number, so it's not known
91
+ whether DIRE dominates total latency so heavily that parallelizing it alongside anything else
92
+ provides only a marginal win, or whether there's a genuine, substantial gain available. Get a real
93
+ number (the script above) before deciding whether extending the thread pool to DIRE/CLIP/
94
+ own-embedding is worth the added complexity β€” the thread-safety blocker is gone, but the
95
+ latency-value question isn't answered yet.
96
 
97
  ## What was actually shipped this round (partial F-17)
98
 
backend/tests/test_dire_detector.py CHANGED
@@ -105,3 +105,55 @@ def test_load_model_is_not_duplicated_under_concurrency(monkeypatch):
105
  )
106
  for d in detectors:
107
  assert d._model_loaded is True
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
105
  )
106
  for d in detectors:
107
  assert d._model_loaded is True
108
+
109
+
110
+ def test_scheduler_is_not_shared_across_instances(monkeypatch):
111
+ """Direct regression test for the scheduler thread-safety fix.
112
+
113
+ DDIMScheduler.set_timesteps()/step()/add_noise() mutate instance
114
+ state -- every DIREDetector instance previously took
115
+ cached_model['scheduler'] directly, meaning every request (a fresh
116
+ DIREDetector each time) shared the exact same scheduler *object*.
117
+ Two DIREDetector instances that both hit the cache (one that just
118
+ loaded it, one on a later cache-hit) must end up with two DISTINCT
119
+ scheduler objects, not two references to the same one -- otherwise
120
+ concurrent DIRE calls racing on set_timesteps()/step() against that
121
+ one shared object is a real bug, not a hypothetical one.
122
+ """
123
+ from backend.services.dire_detector import DIREDetector
124
+ from backend.core.model_cache import get_model_cache
125
+
126
+ get_model_cache().clear()
127
+
128
+ class _FakeScheduler:
129
+ config = {"fake": "config"}
130
+
131
+ @classmethod
132
+ def from_config(cls, config):
133
+ return cls()
134
+
135
+ class _FakePipe:
136
+ def to(self, device):
137
+ return self
138
+
139
+ monkeypatch.setattr(
140
+ "diffusers.DDIMScheduler.from_pretrained",
141
+ staticmethod(lambda *a, **kw: _FakeScheduler()),
142
+ )
143
+ monkeypatch.setattr(
144
+ "diffusers.StableDiffusionPipeline.from_pretrained",
145
+ staticmethod(lambda *a, **kw: _FakePipe()),
146
+ )
147
+
148
+ first = DIREDetector()
149
+ first._load_model() # cold load -- populates the cache
150
+
151
+ second = DIREDetector()
152
+ second._load_model() # cache hit -- must clone, not share
153
+
154
+ assert first.scheduler is not second.scheduler, (
155
+ "two DIREDetector instances share the exact same scheduler object "
156
+ "-- set_timesteps()/step() racing against it under concurrent "
157
+ "DIRE calls would be a real thread-safety bug"
158
+ )
159
+ assert isinstance(second.scheduler, _FakeScheduler) # still the right type/config