DiamondLightSource / DiamondLightSource/smartem-devtools

Ignore prediction models that have been added after acquisiton started

Aperta
#87 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub
Lingua principale
TypeScript
Stelle
0
Fork
0
Metriche di merge delle PR
Nessuna PR unita negli ultimi 30g

Descrizione

> **Needs discussion with Dan** before implementation - the correct locking semantics are a
> product decision, not just a code fix.

## Status

Confirmed present in `smartem-decisions@main`, and **more severe than this issue originally
described**. The original note framed this as future-proofing ("need to think about this");
in fact registering a new prediction model **breaks prediction updates for every in-flight
grid**, via an unhandled exception.

## What actually happens

1. Per-grid model weights are seeded by `initialise_all_models_for_grid`
(`cli/initialise_prediction_model_weights.py:23`), which is called from exactly one place:
`consumer.py:180`, **on grid creation only**. It seeds a row per (grid, model, metric) for
the models that exist at that moment, with `default_weight = 1 / len(models)`.

2. `register_quality_prediction_model.py` contains no grid or weight logic at all. Registering
a new model **does not backfill** weights for grids that already exist.

3. `predictions/update.py:46` re-reads the model list on *every* prior update, unfiltered:

```python
model_rows = (await session.execute(select(QualityPredictionModel))).scalars().all()
```

There is no grid or acquisition scoping.

4. For each model it then requires a per-grid weight row (`predictions/update.py:64`):

```python
.scalars().one()
```

So for any grid created before the new model was registered, the next micrograph hits a model
with no weight row and `.one()` raises `NoResultFound`. This is unhandled, and `prior_update`
is called directly from the RabbitMQ consumer's motion-correction, CTF and particle-picking
handlers (`consumer.py:451`, `:520`, `:583`).

Because there is a CLI whose entire purpose is registering a model, this is a routine
operational action rather than an exotic edge case.

**Not yet established:** what the consumer does with the exception once raised - whether it
nacks, requeues, or drops the message depends on the aio-pika wrapper, which has not been
traced. The unhandled raise in the live event path is confirmed; the blast radius beyond that
point is not.

## Secondary defect

`default_weight = 1 / len(models)` is computed once at grid initialisation. Even if a weight
row were backfilled for a late-registered model, the "weights sum to 1" invariant across a
grid's models would be broken. Any fix needs to state what should happen to normalisation.

## Direction

The original instinct still stands: **lock the set of prediction models for the lifetime of an
acquisition** and have the update path resolve models through that locked set rather than
querying all registered models globally.

Open questions for that discussion:

- Where does the lock live - a join table per grid/acquisition, or derived from the existence
of the seeded weight rows (which would make `update.py` iterate weights rather than models)?
- What should happen to a model registered mid-session: ignored for in-flight acquisitions
(implied by this issue's title) or backfilled at `1/n` with renormalisation?
- Should the update path fail loudly or skip gracefully when a weight row is missing? Today it
fails loudly by accident rather than by design.

Deriving the model set from the seeded weight rows is the smallest change and would fix the
crash and the scoping in one move, but it makes the lock implicit; worth weighing against an
explicit table.

## Code references

- `src/smartem_backend/predictions/update.py:46` - unfiltered global model query
- `src/smartem_backend/predictions/update.py:64` - `.one()` on the per-grid weight row
- `src/smartem_backend/cli/initialise_prediction_model_weights.py:23` - grid-creation seeding
- `src/smartem_backend/consumer.py:180` - sole caller of the seeding
- `src/smartem_backend/consumer.py:451`, `:520`, `:583` - `prior_update` call sites

Guida per i contributori

Nessuna guida per i contributori indicizzata per questo repository

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.