Time series event detection models - #304
samueljackson92 wants to merge 62 commits into
Conversation
2e0ef35 to
90d86b2
Compare
worker.py already calls ray.remote(model_type) when instantiating actors. Having @ray.remote on the class definition too means workers importing tests.models_definitions via class_refs() get an ActorClass, and the second ray.remote() call fails with: TypeError: The @ray.remote decorator must be applied to either a function or a class. Production models (dtw_motif, stumpy_motif, etc.) correctly omit this decorator, so the fix is to align the test mocks with that pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ModelIn.check_model_type validated against the live registry, which crashes in the Models Disabled CI job (no torch → disruption_cnn not registered) and whenever models_definitions.py was imported. ModelIn is a DB-read schema and shouldn't enforce registry membership. E2E tests checked for heading "Train ML Model" but the modal was redesigned to a tabbed layout with heading "ML Models". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ray moved to base dependencies in this branch, so models_dependencies_installed() (which checked for ray) always returned True — models_enabled tests ran in the Models Disabled CI job even without dtaidistance/stumpy/sktime/torch. Fix: check for dtaidistance (an actual models-extra package) instead. Also mark test_new_models.py as models_enabled so it is skipped when the models extras are not installed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Import ray unconditionally in base.py (ray is now a base dep, not optional); the conditional import caused NameError in Models Disabled CI job when models_dependencies_installed() checks dtaidistance instead of ray - Show training success message even when isTrainingActive is true by separating the progress spinner and message into independent conditionals; the old ternary hid "Model training added to job queue!" behind "Training…" the moment trainingStatus was set to "queued" Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ray is now a base dependency so find_spec("ray") always returns True,
meaning models_enabled tests were never skipped in the Disabled CI job.
Check for dtaidistance instead, which is the actual optional models extra.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
test_model_delete_type_version and test_model_load_local_disabled use setup_model_db but lacked the mark, causing UsageError in Disabled CI job. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When trainingModelId is first set, the polling useEffect re-runs and immediately calls fetchModels(). For mock models that train in milliseconds, this overwrites "Model training added to job queue!" with "Training complete!" before Playwright's assertion can see it. Only fire the immediate fetch on initial modal open (trainingModelId is null); let the 5-second interval handle status updates after submission. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Known issue: multiple signal inputs As described in #330, the parameter description says that users can provide multiple signals, but the current schema form only renders one input and provides no way to add another. The Pydantic schema supports multiple values, the problem is in the UI’s list handling. @wk9874 is looking into this. I have also prototyped a possible fix in https://github.com/ukaea/toktagger/blob/slj/ts-event-detection-models-schemaFix/toktagger/ui/src/app/components/ui/schemaForm.tsx |
Guard WorkerRegistry/ActorRegistry class bodies behind models_dependencies_installed() since their unconditional @ray.remote decorator and ray.ObjectRef annotations broke collection of tests/api/models/test_new_models.py (and any other importer of toktagger.api.models.base) whenever ray isn't installed. Also fix get_actor() in worker.py to check for the actual weights file rather than just the results directory, which regressed with the switch to directory-based save/load - the directory can exist (and get_actor thinks the model is trained) even when the weights file inside it is missing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This branch's new DTW/MiniRocket/Shapelet/Stumpy models add several heavy imports (sktime, stumpy, dtaidistance) that toktagger.api.models eagerly loads for every Ray actor worker process, on top of the existing torch import. On the constrained models-enabled CI runner this pushes real (but trivial) train/load/predict round trips past the previous 120s/30s test timeouts, causing GetTimeoutError and stuck-at-202 failures that reproduced deterministically across two CI runs even though everything passes locally. Bump the test-side timeouts to give actor startup enough headroom, and raise the job's overall timeout-minutes so the now-longer unit test step still leaves room for the end-to-end suite that runs after it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
create_sample_predictions() was the only train/load/predict endpoint that didn't call task_registry.update_actors() before dispatching its task, so the model actor it creates was never registered with the ActorRegistry and therefore never killed by the test fixture's per-test teardown (or evicted in production). Confirmed by instrumenting the test fixture with actor/resource counts while constraining Ray to 4 CPUs locally to mirror the CI runner: alive actors and consumed CPU grew without bound across the test file, eventually leaving too little CPU for new actors to schedule, which is what was actually causing the "Pytest (Models Enabled)" CI job's GetTimeoutError/stuck-at-202 failures. Revert the previous timeout bumps (both in the test helpers and the job's timeout-minutes) since they only delayed the same failure - with the leak fixed the actor count stays bounded and the original timeouts are sufficient again, as verified by rerunning the full suite under the same simulated 4-CPU constraint. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Commit 83d7ed1 ("Update UI, multi-class workflow") renamed the train-model dialog's heading from "ML Models" to "Model Training" in modelTrain.tsx, but the e2e assertions checking for that heading weren't updated, so the "Locator expected to be visible" failures on test_model_train_predict/test_timeseries_model_predict were a real, deterministic mismatch rather than flakiness - confirmed by checking the current heading text directly in the component source. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The train-model dialog's model-specific form (schemaForm.tsx's
ModelForm, which contains the "Allocate GPU" switch) only renders
once a model type has been selected and its training schema has
loaded - it's conditionally mounted via {schema && <ModelForm .../>}
in modelTrain.tsx. The GPU switch assertion was still checking
visibility immediately after opening the modal, before any model
type is chosen, so it deterministically failed once the earlier
heading-text mismatch was fixed and the test could reach this point.
Confirmed via the CI accessibility snapshot: the dialog was open with
the model type combobox and name field, but no GPU switch, since no
model was selected yet.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ModelForm (schemaForm.tsx) already handles schema=null gracefully -
it only conditionally renders its nested RJSF form, while the
"Allocate GPU" switch is unconditional. But modelTrain.tsx wrapped
the whole <ModelForm> in an extra {schema && ...} gate, so any model
type with no training params (like mock_timeseries_cnn) never got a
schema and therefore never showed ModelForm at all - hiding a
training option that has nothing to do with a model's custom params.
modelPredict.tsx already renders <ModelForm> unconditionally, which
is the correct pattern; align modelTrain.tsx with it by gating on
selectedModelName instead.
Also fill the newly-required "Model Name" field before submitting
training in the e2e tests - the Train button never enabled without
it, and no existing test filled it in, since it was surfaced by an
element earlier in the flow always failing first.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
form_check() clicks the Train button mid-way through to verify it's blocked by custom-param validation (a missing required "Test String" field). With Model Name filled in only after form_check() returned, that intermediate Train click was blocked by two things at once - the validation form_check is testing, and the still-empty Model Name field - so the button never became enabled and the click hung until Playwright's 30s timeout. Move the Model Name fill before form_check so its click is only gated by what it's actually testing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
test_timeseries_model_predict[mock_timeseries_cnn] intermittently failed with the "disruption_cnn" option never becoming visible after opening the Select Model Type dropdown - the modal's model-types fetch hadn't necessarily resolved yet when the dropdown was opened, and Playwright's default 5s auto-wait wasn't reliably enough margin under CI load. Per this repo's e2e convention, wait for the specific network response the button click triggers instead of relying on an implicit timeout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@abdullah-ukaea this should be ready for re-review now. |
|
As discussed, could we make these two usability improvements before I start the next phase of code review? @samueljackson92
|
…-models # Conflicts: # pyproject.toml # tests/api/routers/test_models.py # toktagger/api/core/worker.py # toktagger/api/routers/meta.py # toktagger/api/static/index.html # toktagger/ui/src/app/components/tools/modelTrain.tsx # uv.lock # zensical.toml
The add-samples directory-listing effect ran for every data schema, so its setShotIds landed after the numerical-range effect and replaced the requested range with whatever parquet files happened to sit under the server's working directory. ShotData projects never render the directory fields at all, so gate the effect on the same file-based schemas the UI gates those fields on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Side Note
In similar spirit to #387 (review) I think this PR includes a few changes beyond just adding the time-series event-detection models. Changes such as multi-file tabular loading could have been handled in a separate PR. Smaller PRs make the intended behaviour clearer, are quicker to review and test, and make things easier for both authors and reviewers.
Summary of review
Thanks for addressing the earlier feedback.
The main areas I think still need attention before merging are:
- I really think model training should probably remain on the project samples page. From an individual shot page, most users will not realise that they are training on all validated project samples. See #304 (comment)
- There are still some time series model correctness edge cases around window sizes, background-only shots, MiniRocket’s minimum window length etc
- Model names are not unique but are used to identify predictions and existing predictions are deleted before replacement predictions succeed.
- I am not convinced the multi-file tabular/Parquet loader combines files correctly. Have you functionally tested it?
- There are many verbose multi-line comments that add unnecessary noise to our codebase. Claude in general loves adding comments so we need to tighten claude.md to combat this.
- This branch is quite stale, I recommend solving the minor merge conflict and pulling the latest dev branch.
The core functionality is more or less done with some bugs, but we really need to do some tidying up before this can be merged.
| @property | ||
| def annotator_name(self) -> str: | ||
| """Name recorded against the annotations this model produces. Models loaded | ||
| from pretrained weights have no name, so fall back to their type.""" | ||
| return self.name or self.type |
There was a problem hiding this comment.
| # Remove any unvalidated predictions from a previous run of this model so | ||
| # that stale annotations don't accumulate in the DB across prediction runs. | ||
| await utils.delete_annotations( | ||
| db_client, | ||
| project_id=project_id, | ||
| sample_id=sample_id, | ||
| created_by=model.annotator_name, | ||
| validated=False, | ||
| ) |
There was a problem hiding this comment.
The previous successful predictions are removed as soon as a new job is submitted. If something fails, the user loses the old results and gets no replacement.
Maybe keep the previous predictions until the new inference has completed successfully. Then you can replace the old predictions as part of the successful write, ideally in one database operation
| ref_idx = max(range(len(signal_data)), key=lambda i: len(signal_data[i][0])) | ||
| ref_time = np.asarray(signal_data[ref_idx][0], dtype=float) | ||
|
|
||
| aligned = [] | ||
| for ta, va in signal_data: | ||
| ta = np.asarray(ta, dtype=float) | ||
| va = np.asarray(va, dtype=float) | ||
| if ta.shape == ref_time.shape and np.array_equal(ta, ref_time): | ||
| aligned.append(va) | ||
| else: | ||
| aligned.append(np.interp(ref_time, ta, va)) |
There was a problem hiding this comment.
I think the shared alignment helper needs more consideration.
This aligns every signal to the longest signal’s full time range, so missing sections are filled with repeated values that could affect training and prediction. It also assumes the time arrays are valid and match their value arrays.
Example: Say you have one signal "Ip" between 0 - 10 seconds and another signal "dalpha" between 2-8 seconds, np.interp fills the missing edges with repeated endpoint values.
Could we validate each signal and align them only over their shared time range, returning a clear error when the data is incompatible?
Examples of potential errors cases:
- Return a clear error for an empty overlap
- Return a clear error for non-increasing or non-finite times
- Return a clear error for mismatched time/value lengths
| for ann in anns: | ||
| ann_time_pairs.append((ann, ta)) | ||
| sample_data.append((ta, va, anns)) | ||
|
|
||
| if not ann_time_pairs: | ||
| raise ValueError( | ||
| f"Signals {params.signal_names} not found in any annotated sample." | ||
| ) | ||
|
|
||
| window_size = compute_window_size(ann_time_pairs) | ||
| logger.info(f"StumpyMotif: inferred window_size={window_size}") | ||
|
|
||
| multivariate = len(params.signal_names) > 1 | ||
| templates: list[tuple[np.ndarray, str]] = [] | ||
| for ta, va, anns in sample_data: | ||
| for ann in anns: | ||
| if not (hasattr(ann, "time_min") and hasattr(ann, "time_max")): | ||
| continue | ||
| if params.class_label and ann.label != params.class_label: | ||
| continue |
There was a problem hiding this comment.
Are you sure other labels should affect the selected class’s window size? This doesn't look right.
The window size is calculated before filtering by class_label, so annotations from other labels can incorrectly affect it. For example, long H-mode annotations could make the window for an ELM model much larger.
Could we calculate the window using only annotations that match class_label, and use all annotations only when class_label is blank?
| f"Signals {params.signal_names} not found in any annotated sample." | ||
| ) | ||
|
|
||
| window_size = compute_window_size(ann_time_pairs) |
There was a problem hiding this comment.
Similar issue to comment https://github.com/ukaea/toktagger/pull/304/changes#r3960474554
To summarise I am concerned that other non selected class labels affect the selected class’s window size.
I think we should filter ann_time_pairs by params.class_label before calculating the window size.
| <View position="fixed" top="size-100" right="size-100" zIndex={9999}> | ||
| <ModelTrainModal project={project} isEnabled={modelsEnabled} /> | ||
| </View> |
There was a problem hiding this comment.
Should model training really be available from the individual sample page?
Opening the training form from one shot suggests that only this shot will be used, but the model actually trains on all validated samples in the project. Nothing in the modal explains that the current shot is not the only training input, so users can easily train on different data from what they expect.
This misleads users when doing model training. Adding training here is a non-trivial UI decision that needs some clear functional justification.
Could we keep training on the samples page like before, or if you insist on keeping it here clearly explain in the modal that all validated project samples will be used?
Personally, I think model training should remain on the samples page like before unless we add a feature to support training on the current shot only.
Project.task is stored as a plain str (use_enum_values=True on ConfiguredModel), so calling .value on it raises AttributeError. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
test_replace_predictions and friends hardcoded type="disruption_cnn", a model only registered when the ray/torch models extra is installed. Register a minimal local mock model instead, since these tests only exercise generic prediction-replacement CRUD logic and don't need a real model. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous fix registered a new mock Model class directly in test_utils.py, but that file has no __init__.py in its directory chain, so pytest imports it as a bare top-level module. Ray pickles the whole ModelRegistry by qualified module path when spinning up worker actors, so the unqualified module name broke unrelated Ray-backed tests in test_models.py with ModuleNotFoundError. Revert to the real disruption_cnn type and instead mark these tests models_enabled, matching the pattern already used elsewhere in this file for tests that need a real registered model type. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Selecting a file type and then filling the directory path fires two overlapping fetches; without cancellation a slower, stale response (e.g. for the previous empty directory path) can overwrite a newer, correct one, intermittently showing "No files found" in CI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>





Adds four new time series event detection models to toktagger:
Each model supports either single or multi-channel inputs.
To Test:
Additionally, check the new tests & CI are passing