Change save/load to use results dir paths instead of single files

[wk9874]

Changes model storage to now make a directory available with name equal to the model ID, so that more than one file can be saved for a given model.

load() now accepts two parameters, the storage directory, and (optionally) a weights file name. If the weights file name is provided, it should take precedence, otherwise fallback to what the model's save() method will have created.

See example implementation in the disruption_cnn model

Note that when loading files via the UI, we will still only support the loading of a single weights file (for now...)

[abdullah-ukaea]

Looks good. I think you need to fix the comment I made in https://github.com/ukaea/toktagger/pull/346/changes#r3705864996 before this can be approved.

the rest are nice to have, nitpicks and some tests that need to be updated. I also noticed some other things which need to be addressed which are not in the git diff.,

[abdullah-ukaea]

Can we check this?

results_dir is meant to be a folder in the model cache, but here it's whatever folder the user's uploaded file came from (e.g. Downloads). A model that ignores the filename and assumes the default name will look in the wrong place. The arguments are also passed positionally instead of by keyword, unlike the rest of the codebase, making them easy to mix up.

Maybe use keyword arguments (results_dir=..., weights_filename=...) to prevent mix-ups, and make sure a model can't end up looking in the wrong place either by documenting this case or having the base class resolve the path itself.

[wk9874]

use keyword arguments (results_dir=..., weights_filename=...)

Done this

results_dir is meant to be a folder in the model cache, but here it's whatever folder the user's uploaded file came from (e.g. Downloads)

That's correct, this function is used for loading pretrained weights from the user's system. So on the UI, they'll provide a path to a file on their system which they want to load, and this will be used to try and load it into the model. If that is successful, then the loaded model will be saved back into the model cache with the model ID, so that future calls can access it normally

A model that ignores the filename and assumes the default name will look in the wrong place.

Yeah this is a bit of an issue, I've documented in the docstring of the model base class that if the filename is provided it should take precedence. I'm not sure what else I can do though to enforce this...

[abdullah-ukaea]

Also not sure but that's fine for now since it fails loudly with FileNotFoundError rather than silently loading the wrong fil the user's upload folder is very unlikely to coincidentally already contain a file called weights.model, so a model that ignores the filename will just error out, not misbehave quietly.

[abdullah-ukaea]

Two small things:

• f"{model.id}" is an unnecessary f-string wrapping a value that's already a string.
• checking exists() then calling rmtree() separately is slightly redundant when it can be done in one safe step.

To fix simplify to shutil.rmtree(model_dir, ignore_errors=True) and drop the f-string.

Note that existing <model_id>.model files from before this change will never be found or cleaned up, so old models silently appear as blank/untrained so maybe document that somewhere? e.g. changelog

[wk9874]

f"{model.id}" is an unnecessary f-string wrapping a value that's already a string.

Done

checking exists() then calling rmtree() separately is slightly redundant when it can be done in one safe step.

I think I prefer the way it is, since it won't suppress unexpected errors, while ignore_errors=True would

Note that existing <model_id>.model files from before this change will never be found or cleaned up

True, will add to changelog when we next do a release

[abdullah-ukaea]

Can we fix VideoCNN?

VideoCNN save() and load() (lines

toktagger/toktagger/api/models/temp.py

Line 95 in 00f2b4b

def save(self, file_stem: str):

and

toktagger/toktagger/api/models/temp.py

Line 98 in 00f2b4b

def load(self, file_path: str):

) were never updated to the new signature, so it'll crash with a TypeError the first time anyone trains this example video model.

update save() and load() here to match this branch's changes, the same way disruption.py was updated?

[abdullah-ukaea]

Can we add tests for these gaps?

No test ever loads a file with a name other than weights.model, so the custom filename logic is never actually tested, and no test checks that a model can save more than one file, even though that's the main point of this change.

Relevant file: https://github.com/ukaea/toktagger/blob/wk9874/models/use_dir_storage/tests/models_definitions.py

These lines are the specific problems.

toktagger/tests/models_definitions.py

Line 47 in 00f2b4b

def load(self, results_dir: pathlib.Path, weights_filename: str | None = None):

and

toktagger/tests/models_definitions.py

Line 106 in 00f2b4b

def load(self, results_dir: pathlib.Path, weights_filename: str | None = None):

Could we add a test that loads a weights file with a different name (e.g. my_weights.pt) and checks the right file was used, and have one mock model save a second file alongside its weights, with a test confirming both files survive a save/load cycle?

[wk9874]

Could we add a test that loads a weights file with a different name (e.g. my_weights.pt) and checks the right file was used

This is covered by test_model_load_local

have one mock model save a second file alongside its weights, with a test confirming both files survive a save/load cycle?
Added to test_model_start_training_params

Will add further tests as part of #319 after this PR is merged, since in that PR I make the functions in worker.py bare functions rather than ray remotes, making them easier to test

[abdullah-ukaea]

ready to be merged, LGTM, nice work on this one.