DiamondLightSource / DiamondLightSource/smartem-devtools

Ignore prediction models that have been added after acquisiton started

オープン
#87 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る
主要言語
TypeScript
スター
0
フォーク
0
PR マージ指標
30日以内にマージされた PR はありません

説明

> **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

コントリビューションガイド

このリポジトリのコントリビューションガイドは索引されていません

調査の方向性

まず src/smartem_backend/predictions/update.py と src/smartem_backend/cli/initialise_prediction_model_weights.py を読み、次に consumer.py にある seeding と prior_update の呼び出し箇所を追ってください。実装前に、acquisition のモデルロックと重みの正規化のセマンティクスを解決してください。完了とは、後から登録されたモデルによって進行中の grid 更新が壊されず、意図された重みの不変条件が維持されることです。

索引モデルが issue の本文から書いたものです。

評価

技術スタック
python, rabbitmq
領域
backend, databases
issue の種類
バグ
難易度
5/5
見積もり時間
1週間以上
活発さ
停滞
明瞭さ
説明が足りない
初心者へのやさしさ
30/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。