Time series event detection models - #304
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 |
|
…maFix Add controls for adding and removing list items in schema forms
|
We have finished the first review of this branch, feel free to address the changes when you are ready @samueljackson92 |
Check for ray instead of dtaidistance to gate optional model imports, unguard torch-based model imports now that the base ray check covers them, and remove ModelRegistry.class_refs()/the string-ref lazy import path in WorkerRegistry.get since it's no longer needed.
…-models # Conflicts: # tests/api/routers/test_models.py # toktagger/api/static/assets/index-BBL3M8gC.css # toktagger/api/static/index.html # toktagger/api/worker.py # uv.lock
DTWMotif/StumpyMotif/MiniRocket load() now takes a results_dir and an optional weights_filename (merged from dev's model-storage refactor) instead of a single file path, so the backward-compat tests need to write into a directory instead of a bare temp file.
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>




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