Config driven detectors - part 3

[ascillitoe]

This is the third 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 the main save and load functionality, and related docs and testing. For more details on the overall config-based save/load strategy, refer to the original PR #389.

Details

The mainstay of this PR is contained in utils/saving.py, and the newly created utils/loading.py.

Additional details:

• The top-level public functions of interest are save_detector and load_detector, both of which have been reworked to write/read config.toml files (in the case of drift detectors). Other detectors are still saved to the legacy .dill format, and support is retained for all detectors to read these legacy files. (to avoid having to regen all remote artifacts immediately).
• Loading functionality has been moved from utils/saving.py to utils/loading.py, since the loading submodule is now larger and is expected to become larger still in the future. A deprecation warning is raised (bit it still works) when from alibi_detect.utils.saving import load_detector is called.
• The backend-specifc bits of saving.py and loading.py have been factored out into tensorflow/_saving.py etc, in preperation for the soon-to-be-built PyTorch/sklearn save/load functionality. This also means the file-wide type: ignore's can be removed.
• The legacy save/load code has been moved to tensorflow/_saving.py and tensorflow/_loading.py, since in reality this was all tensorflow-specific.

Fine details will be given in code comments below.

Ounstanding decisions

• Currently the top-level backend config field is used to declare the backend for backend-specific detectors. But it is also used to set the expected backend for all preprocessing models and kernels etc. Is this OK (at least for now), or do we want specific flags in the model (or preprocess_fn) configs? Part of this decision might depend on whether we ever envisage one library being used for the backend whilst another is used for preprocessing. I can't see this being sensible for PyTorch/Tensorflow, but perhaps for sklearn? - To be addressed in a subsequent PR.

• At the moment the following functions are public:

  • save_detector/load_detector - to save/load detector
  • write_config/read_config - to write/read config.toml from/into config dict
  • validate_config - to validate config dict

  All other functions in saving/loading.py are private, and the tensorflow/_saving.py etc is also private. A number of functions exist to save/load artefact configs, e.g. _load_model_config. Making these public could be useful for some users, e.g. so the model section of a config.toml could be loaded in isolation for debugging, or written runtime model to assist with config.toml generation. However making these public will hinder future code changes, so I'm inclined to leave them private unitl the config functionality is more stable?

• The current options for model type (e.g. 'UAE', 'HiddenOutput', 'custom') have been carried over from the legacy save/load code. We could do with rethinking what is required here.

• Find an example where custom_objects is needed, and investigate how these objects are defined. It might be easier to remove support for this for now. - Removed for now, will be added back in a later PR.

• In resolve_cfg, we only attempt to resolve fields in the config.toml which are listed in FIELDS_TO_RESOLVE. This has the advantage of avoiding infinite recursion, and also allows us to easily tell downstream deps (e.g. MLServer etc) what fields could potentially point to artefacts that will need resolving (such a list has been requested before). However, it is messy, and complicates the resolution of more generic containers such as custom_objects. If we assume that only a validated config will be passed to resolve_cfg (so we are certain of its structure), I'm wondering if we should go back to more generic recursion here? - Left for now. Can change later if there is a good reason.

Post PR TODO's (to be consolidated into issues)

• Adding custom_obj back in. See above bullet, and Config driven detectors - part 3 #469 (comment).
• How to handle warnings when load->save->load across different versions. See Config driven detectors - part 3 #469 (comment).
• Specifying backend for preprocessing and models separately? See above, and Config driven detectors - part 3 #469 (comment).
• Saving/loading state, outlier and adversarial detectors, online detectors, and PyTorch support.
• Improve our registry submodule. See Config driven detectors - part 3 #469 (comment).
• Use enum for backend etc. See Config driven detectors - part 3 #469 (comment).
• Improve the alibi_detect.utils.schemas api page, either with custom templates or using autodoc-pydantic.
• Turn off undoc-members. This will require docstrings to be added to public objects that are currently missing docstrings.
• Python dunder methods to test equality of specific detectors. See Config driven detectors - part 3 #469 (comment).
• Investigate randomness with Tensorflow and Pytorch based detectors, so that we can properly test the loaded and original detector predictions. See Config driven detectors - part 3 #469 (comment).
• Run isort on entire code base, and consider adding a check to CI.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[ascillitoe]

• The current options for model type (e.g. 'UAE', 'HiddenOutput', 'custom') have been carried over from the legacy save/load code. We could do with rethinking what is required here.

The model type field wasn't that intuitive for use in the detector config (it was not clear why the type had to be UAE if an embedding was also spec'd, and the detector could not be saved when an embedding model was used with an additional model (as is done in the amazon example). Use of the HiddenOutput type was also not particuarly intuitive.

The model type field has now been removed, and replaced with simpler logic:

• When an embedding and model are spec'd, the model and embedding are automatically coupled in a UAE object, with the need for the user to specify this in the config.
• Previously, spec'ing type='HiddenOutput' gave the same load behaviour as type='custom'. However, it would be desirable for HiddenOutput to accept a layer field, so that a full model could be spec'd in the config, along with the hidden layer to extract. This is now achieved with a new layer field - if layer is given, the spec'd model is wrapped in a HiddenOutput, and the requested layer is extracted.

Note

One side effect of the current HiddenOutput implementation is that if a config.toml is loaded with layer set, and the detector then saved again, the new config.toml will not contain the layer field. Instead, the model it references will be the extracted HiddenOutput model. It would be desirable to save the original model and layer field instead, as this would allow later configuration (i.e. change the extracted layer etc). However, this is not possible with the current HiddenOutput implementation since the original model is not stored.

[ascillitoe]

• Find an example where custom_objects is needed, and investigate how these objects are defined. It might be easier to remove support for this for now.

custom_objects is a dict to be passed to TensorFlow's load_model. Internally, we only currenty use it in one place:

alibi-detect/alibi_detect/utils/fetching.py

Lines 82 to 86 in 6b5b2c7

	if dataset == 'cifar10' and model == 'resnet56':
	custom_objects = {'backend': backend}
	else:
	custom_objects = None
	clf = tf.keras.models.load_model(save_path, custom_objects=custom_objects)

but in general, it should be capable of passing custom objects/functions (e.g. for custom layers) to TensorFlow models. This is introduces another challenge; how do we save (and load) arbitrary custom objects stored inside custom_objects. We could support spec'ing the custom objects as runtime regsistered objects, as well as loading the custom objects from .dill (assuming they can be serialized as dill).

Note

This still needs a little more thought. It is also going to be a point of difference between TensorFlow and PyTorch models.

[jklaise]

This means the whole section is commented out?

[ascillitoe]

I've just removed the "Advanced usage" header, and promoted "Validating config files", since we didn't have any other level 3 headings under "Advanced usage". Can change in future if we add more "Advanced topics".

[ascillitoe]

@ascillitoe on the submodules for save_detector, load_detector, I think it's about time we exported these into a higher public API rather than the user having to import from alibi_detect.utils.saving/loading? In alibi I've just done alibi.saving, if we wanted to do the same perhaps a new subpackage alibi_detect.saving could be defined that imports the public functions from the deeper modules?

@ascillitoe also thinking if we should elevate the registry from alibi_detect.utils.registry to e.g. alibi_detect.registry?

alibi_detect.utils.validate, alibi_detect.utils.registry, alibi_detect.utils.saving, and alibi_detect.utils.loading have now all been moved to alibi_detect.saving. For backwards compatiblity I've left save_detector and load_detector wrapper functions in alibi_detect.utils.saving with DeprecationWarning's.

[ascillitoe]

@ascillitoe quite a few modules under utils (including but not limited to saving.py and loading.py are not rendering on the API docs.

This has been fixed by changing to sphinx-autoapi. It was due to problems with the pydantic api page not compiling properly when tensorflow etc were mocked with autodoc. #482 concerns the switch from autodoc to sphinx-autoapi. In the PR here I've made the same changes, but haven't haven't updated all the broken links to api docs since they are dealt with in #482, and hence will be resolved when we merge config_driven_detectors into master.

[ascillitoe]

@ascillitoe can I suggest running isort at least on the new modules? I know we don't have it in the official guidelines, but perhaps we should...

I've just run this on alibi_detect.saving. Otherwise too many changes! Have added note to open an issue too.

[ascillitoe]

@ascillitoe a thought I have, if at any point in saving an error is raised then it seems there would be a half-baked saved detector on disk. Should we wrap our top-level saving in try/except and do a clean-up if the saving is unsuccessful for any step?

I've added something to delete all new files (not ones already in filepath) and delete the filepath directory if its empty. Also added a test test_cleanup.

[ascillitoe]

@jklaise @mauicv thanks for all your comments! I've tried to address them all (I've left some unresolved where they need reviewing).

[mauicv]

LGTM

[jklaise]

LGTM, but can you remind me if changes to imports using alibi.saving/loading as opposed to alibi.utils.saving/loading were already made in the example notebooks?

[ascillitoe]

LGTM, but can you remind me if changes to imports using alibi.saving/loading as opposed to alibi.utils.saving/loading were already made in the example notebooks?

Thanks, I had missed those. I've now updated all mentions and uses to alibi_detect.saving in docs and examples.