SIENTIAPDE-1646
Update E2E test report and enhance drift analysis handling - Updated the E2E test report metrics to reflect the latest test results, showing 47 collected tests with all passing. - Removed outdated sections related to failed tests and their causes, streamlining the report. - Implemented a regression fix in the drift analysis to handle empty merged frames, ensuring workflows skip export when no drift metrics are available. - Enhanced the `insert_sample_data` and `insert_sample_prediction` functions to allow customizable timestamps for better test accuracy. - Refactored E2E tests to improve clarity and maintainability, particularly in handling repeat scenarios with distinct timestamps.
This commit is contained in:
vitor-aignosi
@@ -8,142 +8,41 @@
|
||||
|
||||
| Metric | Count |
|
||||
|--------|------:|
|
||||
| Collected | 43 |
|
||||
| **Passed** | **37** |
|
||||
| **Failed** | **6** |
|
||||
| Collected | 47 |
|
||||
| **Passed** | **47** |
|
||||
| **Failed** | **0** |
|
||||
|
||||
Full pytest output (compressed by `rtk`) was written to:
|
||||
|
||||
`~/.local/share/rtk/tee/1778268193_pytest.log`
|
||||
Full pytest output (when using `rtk`) is stored under `~/.local/share/rtk/tee/` as timestamped `*_pytest.log` files.
|
||||
|
||||
---
|
||||
|
||||
## Failed tests (6)
|
||||
## Regression fixed during this run (drift)
|
||||
|
||||
1. `e2e/test_drift.py::test_drift_happy_path_persists_all_columns_with_reference_data`
|
||||
2. `e2e/test_drift.py::test_drift_uses_30pct_fallback_when_reference_unavailable`
|
||||
3. `e2e/test_drift.py::test_drift_chunk_period_seconds_preserves_seconds_in_chunk_start_date`
|
||||
4. `e2e/test_predictions_batch_prediction_process.py::test_scenario_2_1_3_input_gate_triggers_repeat`
|
||||
5. `e2e/test_predictions_batch_prediction_process.py::test_scenario_2_2_3_transform_gate_triggers_repeat`
|
||||
6. `e2e/test_predictions_batch_prediction_process.py::test_scenario_2_3_3_predict_gate_triggers_repeat`
|
||||
An initial e2e run failed **3** drift tests with:
|
||||
|
||||
**Follow-up:** Jensen–Shannon NULLs for the drift happy-path scenario are fully traced (histogram out-of-range + `density=True`, vs NannyML’s leftover bin) in [DRIFT_JS_NULL_INVESTIGATION.md](./DRIFT_JS_NULL_INVESTIGATION.md).
|
||||
`ValueError: The truth value of a Index is ambiguous`
|
||||
|
||||
**Cause:** `calculate_drift` passes `reference_data.columns` (a **pandas `Index`**) into `ModelMetrics.get_drift_metrics`, which forwards it to `sientia_model.analytics.drift_analysis.DriftAnalysis`. The analyzer uses patterns such as `if not features:` on the feature list. Boolean evaluation of an `Index` raises in pandas.
|
||||
|
||||
**Fix (in `laborious/activities/model_metrics.py`):** At the start of `get_drift_metrics`, normalize with `feature_names: list[str] = list(reference_columns)` and use `feature_names` in the `DriftAnalysis` config and in `detect_univariate_drift` / `detect_multivariate_drift`.
|
||||
|
||||
After this change, the full **`e2e/`** suite was re-run and **all 47 tests passed**.
|
||||
|
||||
---
|
||||
|
||||
## Failure group A — Drift: `drift_metrics.value` NOT NULL (2 tests)
|
||||
## Suite coverage (high level)
|
||||
|
||||
### Error
|
||||
|
||||
`psycopg2.errors.NotNullViolation`: null value in column `value` of relation `sientia_data.drift_metrics` violates not-null constraint.
|
||||
|
||||
Example failing row (from logs): `feature=sensor_2`, `method=jensen_shannon`, `value=null`, with `kolmogorov_smirnov` / `wasserstein` populated for the same chunk.
|
||||
|
||||
The bulk INSERT built by `Activities.export_data_to_postgres` includes parameters such as `'value__4': None` for `jensen_shannon` on a given chunk.
|
||||
|
||||
### Root cause
|
||||
|
||||
`calculate_drift` (real `DriftAnalysis` + `ModelMetrics.get_drift_metrics`) can emit **NaN / missing** values for some metric methods (here **Jensen–Shannon**) on some features/chunks. Pandas/SQLAlchemy turns that into SQL `NULL`, while the E2E schema (mirroring production) defines:
|
||||
|
||||
```sql
|
||||
value numeric NOT NULL
|
||||
```
|
||||
|
||||
in `e2e/db_schema.sql` for `sientia_data.drift_metrics`.
|
||||
|
||||
### Recommended fixes (pick one consistent with product rules)
|
||||
|
||||
1. **Application layer (preferred if NULLs are never valid in production):** Before export, sanitize the drift dataframe — e.g. drop rows where `value` is null/NaN, or replace with a defined sentinel (only if product agrees), or skip emitting that method row when the statistic is undefined.
|
||||
2. **Analytics layer:** Harden the Jensen–Shannon (and similar) paths so they always return a finite float for the supported inputs, or explicitly map “undefined” to an agreed numeric convention.
|
||||
3. **Schema (only if product allows missing metrics):** Align DDL with reality by making `value` nullable — **only** if production and downstream consumers already expect missing metrics; the E2E comment in `test_drift.py` suggests `feature`/`timestamp` nullable cases exist, but `value` is still listed in `NON_NULL_DRIFT_COLUMNS`.
|
||||
|
||||
### Tests affected
|
||||
|
||||
- `test_drift_happy_path_persists_all_columns_with_reference_data`
|
||||
- `test_drift_uses_30pct_fallback_when_reference_unavailable`
|
||||
| Area | File(s) | Notes |
|
||||
|------|---------|--------|
|
||||
| Drift workflow | `e2e/test_drift.py` | Happy path, 30% reference fallback, empty target, bad `chunk_period`, empty merge / no export, sub-minute chunking |
|
||||
| Predictions batch | `e2e/test_predictions_batch_*.py` | Main workflow, prediction process gates / repeat, format export |
|
||||
| Child workflows | `e2e/test_child_workflows_e2e.py` | Format + export path |
|
||||
| Minimal retrain | `e2e/test_minimal_retrain.py` | Success / failure / missing target / no data |
|
||||
| MinIO offload | `e2e/test_minio_offload.py` | Load query + batch path |
|
||||
| Simple metrics | `e2e/test_simple_metrics.py` | Persistence, subset, edge cases |
|
||||
|
||||
---
|
||||
|
||||
## Failure group B — Drift: chunk period seconds (1 test)
|
||||
## Relation to earlier reports
|
||||
|
||||
### Error
|
||||
|
||||
`AssertionError: expected at least one drift row to be persisted` (`e2e/test_drift.py:493`).
|
||||
|
||||
The workflow run completed without failing the test via `WorkflowFailureError`, but **no rows** were found in `sientia_data.drift_metrics` for the model.
|
||||
|
||||
### Likely cause
|
||||
|
||||
In `Drift.run`, persistence runs only when `if drift_data:` is truthy (`laborious/workflows/drift.py`). An **empty** drift result skips `export_data_to_postgres`, so the table stays empty.
|
||||
|
||||
Probable reasons:
|
||||
|
||||
- With **`chunk_period='s'`** and only **three** target rows (30 s spacing), `calculate_drift` / `DriftAnalysis` may produce **no output rows** (insufficient data per chunk or internal filters).
|
||||
- Less likely here: time-window mismatch — timestamps are built from `datetime.now(UTC)` with `interval: 60` minutes from `drift_base.json`, so data should still fall in the window.
|
||||
|
||||
### Recommended fixes
|
||||
|
||||
1. **Test data:** Increase the number of second-spaced points (and/or span multiple chunk boundaries) so the analyzer reliably emits at least one chunk row.
|
||||
2. **Product code:** If sub-minute chunking is required to always produce metrics when any data exists, adjust `ModelMetrics` / `DriftAnalysis` integration for small-N second buckets.
|
||||
3. **Diagnostics:** Run the same scenario with `pytest -s` and confirm logs for “empty `drift_data`” vs export errors.
|
||||
|
||||
Solução a ser aplicada:
|
||||
Dividir em dois testes:
|
||||
|
||||
1. Teste com dados suficientes para produzir pelo menos uma linha de drift
|
||||
2. Teste com dados insuficientes para produzir pelo menos uma linha de drift, mas ja esperando os erros e validando que nao foi persistido nada
|
||||
|
||||
### Test affected
|
||||
|
||||
- `test_drift_chunk_period_seconds_preserves_seconds_in_chunk_start_date`
|
||||
|
||||
---
|
||||
|
||||
## Failure group C — Prediction REPEAT path: unique constraint on predictions (3 tests)
|
||||
|
||||
### Error
|
||||
|
||||
`psycopg2.errors.UniqueViolation`: duplicate key value violates unique constraint **`unique_model_id_timestamp`** on `sientia_data.predictions` (`model_id`, `timestamp`).
|
||||
|
||||
### Root cause
|
||||
|
||||
REPEAT is handled by `Activities.repeat_last_prediction` (registered from `sientia_do.temporal.activities.postgres_sync.Postgres`, via `Storage` inheritance in `laborious/activities/storage.py`). The E2E tests seed a prior row with a **fixed** timestamp:
|
||||
|
||||
```python
|
||||
# e2e/test_predictions_batch_prediction_process.py — insert_sample_prediction
|
||||
VALUES ({model_id}, '2024-01-01 12:00:00+00:00', ...)
|
||||
```
|
||||
|
||||
The helper `assert_repeat` expects **two** rows with the same `(model_id, prediction, prediction_confidence, prediction_status)` but compares **only** those columns — not `timestamp` (`e2e/helpers.py`). So the intended behavior is: **duplicate business payload**, not necessarily **duplicate primary unique key** `(model_id, timestamp)`.
|
||||
|
||||
If `repeat_last_prediction` **INSERT**s a copy using the **same** `timestamp` as the last prediction, Postgres correctly rejects the second insert.
|
||||
|
||||
### Recommended fixes
|
||||
|
||||
1. **Implement REPEAT as UPSERT:** Use `ON CONFLICT (model_id, timestamp) DO UPDATE` (or the project’s existing `export_data_to_postgres` `on_conflict` pattern used in `FormatAndExportPrediction`) when writing the repeated prediction — if the product definition of REPEAT is “refresh same logical slot.”
|
||||
2. **Insert with the current batch timestamp:** Copy numeric/status fields from the last row but set `timestamp` to the **new** batch instant (e.g. the workflow’s `last_timestamp` / slice timestamp). This matches `assert_repeat`, which does not assert on `timestamp`.
|
||||
3. **Test-only change (weakest):** Relax assertions or change seed data — only if production behavior is “duplicate key is expected” (unlikely).
|
||||
|
||||
Solução a ser aplicada:
|
||||
Alterar o teste para usar o timestamp da ultima execucao do batch, ao inves do timestamp do primeiro batch.
|
||||
2. **Insert with the current batch timestamp:** Copy numeric/status fields from the last row but set `timestamp` to the **new** batch instant (e.g. the workflow’s `last_timestamp` / slice timestamp). This matches `assert_repeat`, which does not assert on `timestamp`.
|
||||
|
||||
### Tests affected
|
||||
|
||||
- `test_scenario_2_1_3_input_gate_triggers_repeat`
|
||||
- `test_scenario_2_2_3_transform_gate_triggers_repeat`
|
||||
- `test_scenario_2_3_3_predict_gate_triggers_repeat`
|
||||
|
||||
---
|
||||
|
||||
## Passing areas (sanity check)
|
||||
|
||||
All scenarios in `test_child_workflows_e2e.py`, `test_minimal_retrain.py`, `test_minio_offload.py`, `test_predictions_batch_format_export.py`, `test_predictions_batch_main_workflow.py` (except the three REPEAT cases above), and `test_simple_metrics.py` **passed** in this run.
|
||||
|
||||
---
|
||||
|
||||
## Suggested order of work
|
||||
|
||||
1. Fix **drift `value` NULL** — unblocks two high-value drift E2Es and may clarify the chunk-seconds scenario if exports start succeeding consistently.
|
||||
2. Fix **`repeat_last_prediction` uniqueness** — unblocks three prediction-process E2Es; implementation likely lives in **`sientia_do`** Postgres activities, not in this repo.
|
||||
3. Revisit **`test_drift_chunk_period_seconds`** data volume / expectations after drift export is stable.
|
||||
Older failures described in previous versions of this document (e.g. Jensen–Shannon NULLs vs `drift_metrics.value` NOT NULL, sparse `chunk_period='s'` data) are **not** reproduced in this run. If those topics resurface after data or dependency changes, see the dedicated notes under `docs/` (e.g. drift / JS investigations) and `e2e/scenarios.md`.
|
||||
|
||||
Reference in New Issue
Block a user