Config driven detectors - part 4

[ascillitoe]

This is the fourth (and final!) part of a series of PR's for the config-driven detector functionality. The original PR (#389) has been split into a number of smaller PR's to aid the review process.

Summary of PR

This PR implements a number of final fixes and refactorings:

1. The preprocess_at_init and preprocess_at_pred logic implemented in Preprocess poc #381 and Config driven detectors - part 1 #458 has been reworked. This turned out to have a problem in how it dealt with update_x_ref, since regardless of x_ref_preprocessed, we still need to update reference data within .predict() when update_x_ref is set. All offline drift detectors have been reworked to use the old logic (but with preprocess_x_ref renamed preprocess_at_init), with the addition that self.x_ref_preprocessed is also checked internally.
2. The previous get_config methods involved a lot of boilerplate to try to recover the original args/kwargs from detector attributes. The new approach calls a generic _set_config() with __init__, and then self.config is returned by get_config. This should significantly reduce the workload to add save/load to new detectors. To avoid memory overheads, large artefacts such as x_ref are not set at __init__, and instead are added within get_config.
3. Owing to the ease of implementation with the new get_config approach, save/load has been added for the model uncertainty and online detectors!
4. Kernels and preprocess_fn's were previously resolved in _load_detector_config, which wasn't consistent with how other artefacts were resolved (it also caused added extra challenges). These are now resolved in resolve_config instead. Following this the KernelConfigResolved and PreprocessConfigResolved pydantic models have been removed (they could be added back but it would complicate resolve_config).
5. Fixing determinism in Random seed utilities #496 has allowed us to compare original and loaded detector predictions in test_saving.py. This uncovered bugs with how kernels were saved and loaded. These have been fixed.
6. The readthedocs.yml has been fully updated to the V2 schema so that we can use Python 3.9 for building the docs. This is required as the class NDArray(Generic[T], np.ndarray[Any, T]) in utils._typing causes an error with autodoc on older Python versions.

Future TODO's

• The pydantic schema API docs need follow-up work (to be done in a separate PR to feature/config_driven_detectors). The types aren't currently visible for model fields, because fields are actually class attributes and sphinx_autodoc_typehints doesn't extract types from attributes. Possible fixes include documenting each attribute in the docstring (but then fields will be documented twice unless we turn off :undoc-members:, or using autodoc-pydantic (this might require Install alibi-detect during RTD docs build #499).
• Add a dev guide on creating new detectors (i.e. how to add _set_config() for backend vs non-backend detectors etc) to the github wiki.
• Update the "Saving and Loading" sections of docs methods pages.
• Collate all future config related TODO's into an issue (or issues). There are a number to be collected from WIP: Config driven detectors #389 and Config driven detectors - part 3 #469, . Those outstanding from this PR are:
  • Tidy LSDDDrift normalization logic: Config driven detectors - part 4 #497 (comment)

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[ascillitoe]

Thanks for all the helpful comments @jklaise! I've had a first pass at them now.

[ascillitoe]

P.s. @jklaise, these sections in the docs methods pages need updating:

I've added a TODO in the PR description to do this before merging into master, as don't want to add to the file diff in this PR.

[jklaise]

LGTM given the discussion from the previous review.

[mauicv]

will this duplicate the memory during init even if it's later gc'd?

[ascillitoe]

You have a point... looking into this...

[ascillitoe]

OK you were totally correct. The above implementation would not avoid x_ref duplication as intended. We couldn't remove .copy() as this leads to backend etc in the config being altered by the later backend-specific operations. I've now moved set_config to the top of the __input__ for all detectors with a backend, which also brings them into parity with the non-backend detectors.

This required a minor change to how we get version for the config in _set_config, since self.meta is no longer set when _set_config is called:

            'meta': {
                'version': self.meta['version'],
                'config_spec': __config_spec__,
            }

changed to:

            'meta': {
                'version': __version__,
                'config_spec': __config_spec__,
            }

Don't think this is as an issue because we import __version__ in alibi_detect.base anyway...

[ascillitoe]

P.s. IMO the above complexity is a symptom of the way we set detector backends i.e. self._detector = MMDDriftTF(...). It is slightly odd how MMDDrift is not a detector itself, but only a container with a detector as an attribute.

I've been discussing with @mauicv how we might want to refactor this, and will add some ideas to #512.

[mauicv]

Do you iterate through recursively and if it's a string you prepend the directory and see if it exists in the file system? I'm trying to think if there's some way this might backfire?

[ascillitoe]

Yes exactly that. Then only update the string in the config (with the new prepended file path) if it is a file or dir.

The two ways I can think of which it might backfire are:

1. If a config contains a field which a str but shouldn't be a filepath (e.g. a regsitry str), whose string is such that when prepending the config filepath to it the new string is actually a filepath. This field would then incorrectly be updated. I can't think of a realistic example of where this would happen, but it is theoretically possible.
2. The bigger disadvantage is that if a filepath has been spec'd in the config but it is missing/incorrect, it won't be updated with the prepended path. The FileNotFoundError error raised later would then be confusing if it prints out the problem directory. The easiest thing to do is probs just go through and check no FileNotFoundError's print out the given directory...

[ascillitoe]

Following up on this, I've actually decided not to remove the filepath included in FileNotFoundError's such as that in saving.tensorflow._loading.load_model:

raise FileNotFoundError(f'No .h5 file found in {model_dir}.')

Although model_dir wouldn't have the config file dir prepended to it (see 2. above), the filepath would still match that in config.toml, which might actually be a nice feature to aid the user's debugging...

@mauicv shout if you disagree, otherwise think we're good to go!

[mauicv]

We should accumulate all these todo's in an issue somewhere once this is merged so that when stuff gets fixed we know what to update

[mauicv]

LGTM!