Skip to content

feat: expose prediction setup id in prediction responses (CLIM-1110) - #607

Open
edvinstava wants to merge 6 commits into
masterfrom
eas/feat/clim-1110-prediction-setup-id
Open

edvinstava wants to merge 6 commits into
masterfrom
eas/feat/clim-1110-prediction-setup-id

Conversation

@edvinstava

@edvinstava edvinstava commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Adds predictionSetupId to prediction responses so dashboard widgets can link to the prediction in the modeling app. Returns null when no setup is attached.

@mortenoh

Copy link
Copy Markdown
Contributor

Reviewed the diff. No live correctness bug: every production path returns an ORM row with the FK loaded, so predictionSetupId serializes correctly on all four PredictionInfo endpoints. The findings below are about fragility and test cost.

Worth fixing before merge

1. The FK has no ON DELETE SET NULL in the ORM (chap_core/database/tables.py:338)

SET NULL only exists in alembic e5f6a7b8c9d0. A fresh install runs create_all and then stamps at a7b8c9d0e1f2, which is after that migration, so fresh Postgres gets a plain FK, and so does every test schema. Deleting a setup works today only because prediction_setup_service.py:224 uses session.delete(setup) and the relationship has no passive_deletes, so the ORM nulls the children. The comment at prediction_setup_service.py:218 saying the DB enforces it is wrong. Any out-of-band delete leaves a prediction pointing at a 404 setup. Suggest sa_column_kwargs={"ondelete": "SET NULL"} on the field so create_all matches the migration, and fix the comment.

2. prediction_setup_id is declared twice (tables.py:338 and :351)

Two copies with two different descriptions, so OpenAPI documents one column two ways. PredictionBase exists for fields shared by the row and the read views, and already carries dataset_id as an FK on the non-table base. Move the field there and drop both per-class copies. The mapped column is unchanged, so no migration.

3. default=None on the read field (tables.py:351)

It is the only default on any PredictionInfo field. With from_attributes, a missing source attribute serializes as a silent null instead of a validation error. Nullability is already expressed by int | None; drop the default.

Test coverage

4. The only test that proves a setup-triggered run populates the field on the wire is skipped (test_db_endpoints.py:1430, test_run_prediction_setup_full_flow). The new test sets prediction.prediction_setup_id by hand, so a dropped kwarg anywhere in /run -> apply_async -> run_prediction -> add_predictions would keep every test green while scheduled predictions return null. Either fix that test's fixture or add a /run test that captures the queue_db kwargs.

5. Two of the four serialization paths have no assertion on the new field: GET /v1/jobs/{job_id}/prediction_result (test_jobs_routes.py:143) and PredictionSetupReadWithPredictions.predictions (test_db_endpoints.py:885, which already has setup_id in scope). One-line assertions in each.

Test cost

6. The 2x2 parametrize seeds the full database four times (test_db_endpoints.py:905). The linked_setup=False axis only asserts the fixture's default state. test_backtest_info_exposes_prediction_setup_id_when_setup_exists at line 1089 already uses the cheaper pattern: assert None, create the setup, assert the id, in one run.

7. Third verbatim copy of the create-setup-and-link-prediction block (test_db_endpoints.py:917; also at 886 and 1000). A _link_prediction_to_setup helper next to _create_prediction_setup at line 789 would serve all three.

Process: the PR is not in draft, which the repo convention expects until it is ready for review.

mortenoh and others added 5 commits September 15, 2026 12:45
Move prediction_setup_id onto PredictionBase so the column and the read
view share one declaration instead of two with diverging descriptions.
The mapped column is unchanged, so no migration is needed.

Collapse the 2x2 parametrize on the new test to a single run per endpoint
that asserts null, links a setup, then asserts the id. Add assertions on
the two remaining serialization paths (jobs prediction_result and
PredictionSetupReadWithPredictions.predictions) and pin the
prediction_setup_id kwarg passed from /run to queue_db.
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