Feature Optional backends functionality #538

mauicv · 2022-06-21T13:52:22Z

Todo:

What is this

This work branches off feature/dep-management and will make tensorflow and pytorch optional dependencies of alibi-detect.

Two main objectives are:

1. Make `tensorflow` and `pytorch` optional dependencies:

I've combined these into this single PR because they're all quite interdependent and so It was hard to do them separately without writing code to manage functionality implemented in one but not the others.

2. Add BackendValidation for all detectors:

This wasn't such an issue in alibi as only CFRL implemented multiple backends. In detect however there are multiple cases where we have a tensorflow, PyTorch or sklearn backend and on top of this sometimes the tensorflow backend is dependent on tensorflow-probability as well. Previously this was handled using duplicated code in each detector. I've added a BackenValidator that replaces this and issues more specific error messages for optional dependencies.

What this PR doesn't include:

The install options in the docs. I think it makes sense to do this in a later PR on the feature branch for all the optional deps all at once, once this branch has been merged in.
The install options in the README.md for same reason as 1.
The config-driven work in the release branch. This will be done in a separate PR in order to manage Release/Master branch complexity issues.
Make Licenses updates. This will be done in a PR on the feature branch.

Notes:

Circular imports:

I'm not sure why this wasn't an issue with alibi but in alibi-detect often we have the following scenario:
- Import model from models
- In importing the above model it needs a function from utils.distance
- Importing this function means the code in utils.__init__ is executed
- This code imports utils.fetching which in turn imports all the models from models
- We get a circular import error

This is an issue that's introduced by us trying to bundle all the functionality into the init file and the correct approach is further scoping and modularization. The solution to the above is a utils/fetching/__init__.py as separate from the utils/__init__.py

Changing import statements:

This PR results in lots of changes to the notebooks primarily in the form of changes to the notebooks to reflect the new intended public API. Note that these shouldn't be breaking changes as the objects in question should still be in the old locations the only difference is that where they need to be protected from ImportErrors we import them into the __init__.py file in a module instead of from the file directly. So old code will still run but the object imported won't be protected as per import_optional. This might mean code will break if users are running default installs as they won't have all the relevant dependencies. Any release should incorporate a note that a quick fix is updating their installs to pip install alibi[x,y,z] for relevant x, y, z. A longer fix is to update the import statements to the public API. Most of the time this won't even be necessary, just where imports have changed as:

from alibi_detect.utils.pytorch.kernels import DeepKernel

to

from alibi_detect.utils.pytorch import DeepKernel

The full list of changes is as follows:

from alibi_detect.utils.tensorflow.kernels import DeepKernel -> from alibi_detect.utils.tensorflow import DeepKernel
from alibi_detect.utils.tensorflow.prediction import predict_batch -> from alibi_detect.utils.tensorflow import predict_batch
from alibi_detect.utils.pytorch.data import TorchDataset -> from alibi_detect.utils.pytorch import TorchDataset
from alibi_detect.models.pytorch.trainer import trainer -> from alibi_detect.models.pytorch import trainer
from alibi_detect.models.tensorflow.resnet import scale_by_instance -> from alibi_detect.models.tensorflow import scale_by_instance
from alibi_detect.models.tensorflow.resnet import scale_by_instance -> from alibi_detect.models.tensorflow import scale_by_instance
from alibi_detect.utils.pytorch.kernels import DeepKernel -> from alibi_detect.utils.pytorch import DeepKernel
from alibi_detect.models.tensorflow.autoencoder import eucl_cosim_features -> from alibi_detect.models.tensorflow import eucl_cosim_features
from alibi_detect.utils.tensorflow.prediction import predict_batch -> from alibi_detect.utils.tensorflow import predict_batch
from alibi_detect.models.tensorflow.losses import elbo -> from alibi_detect.models.tensorflow import elbo
from alibi_detect.models import PixelCNN -> from alibi_detect.models.tensorflow import PixelCNN
from alibi_detect.utils.tensorflow.data import TFDataset -> from alibi_detect.utils.tensorflow import TFDataset
from alibi_detect.utils.pytorch.data import TorchDataset -> from alibi_detect.utils.pytorch import TorchDataset

also see this related comment!

…datasets

…functionality

review-notebook-app · 2022-06-21T13:52:27Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

ascillitoe · 2022-06-28T09:20:16Z

Noting down a few thoughts:

Number of optional deps groups: The number is getting quite large, and might grow further in future. We might want to reassess the motivation behind each group. Specifically, is the numba group needed, and can we merge tensorflow and tensorflow-probability (apologies, separating the last two was largely down to my initial input, which I am now thinking was maybe misguided).
conda optional deps groups: We need to consider what is included (in run) and what is optional (in run_constrained) in our conda recipe. This is made more complex by the need to consider what is included in recipes of our dependencies i.e. the transformers recipe includes pytorch, so we cannot have pytorch as optional in our conda recipe. Based on offline discussion with @mauicv, the latest thinking here is that we should keep our conda recipe "fat" for now (i.e. include tensorflow and pytorch), and only have fbprophet as an optional addition here. Looking at how to slim down the conda recipe and speed up the install should be kept as a separate task Revisit conda recipe #443.

mauicv · 2022-06-28T10:11:32Z

w.r.t.

Number of optional deps groups: The number is getting quite large, and might grow further in future. We might want to reassess the motivation behind each group. Specifically, is the numba group needed, and can we merge tensorflow and tensorflow-probability (apologies, separating the last two was largely down to my initial input, which I am now thinking was maybe misguided).

Do you mean having a tensorflow bucket and a TensorFlow-probability bucket that also has tensorflow or just having a single tensorflow bucket which includes tensorflow-probability? I think I'm generally in favour of the former.

conda optional deps groups: We need to consider what is included (in run) and what is optional (in run_constrained) in our conda recipe. This is made more complex by the need to consider what is included in recipes of our dependencies i.e. the transformers recipe includes pytorch, so we cannot have pytorch as optional in our conda recipe. Based on offline discussion with @mauicv, the latest thinking here is that we should keep our conda recipe "fat" for now (i.e. include tensorflow and pytorch), and only have fbprophet as an optional addition here. Looking at how to slim down the conda recipe and speed up the install should be kept as a separate task Conda install stuck on "Solving environment" #443.

agreed!

…functionality

README.md

alibi_detect/cd/model_uncertainty.py

alibi_detect/datasets.py

alibi_detect/models/tensorflow/__init__.py

ascillitoe · 2022-07-04T15:19:29Z

alibi_detect/models/tensorflow/__init__.py

@@ -11,7 +47,14 @@
    "VAE",
    "VAEGMM",
    "resnet",
+    "scale_by_instance",


Just to double check; is scale_by_instance promoted from tensorflow.resnet to tensorflow so that we can do the import_optional check in __init__? This makes sense I think, although a minor point is that we would ordinarily want to raise a DeprecationWarning if the old import usage from alibi_detect.models.tensorflow.resnet import scale_by_instance is used. Perhaps not important in this case as scale_by_instance is not used a lot...

Yeah, that's exactly correct. I'll add the relevent DepreciationWarnings

see this related comment!

Leaving this one "unresolved" for visibility...

alibi_detect/utils/fetching/fetching.py

alibi_detect/utils/frameworks.py

alibi_detect/utils/perturbation.py

ascillitoe · 2022-07-04T15:39:27Z

alibi_detect/utils/pytorch/__init__.py

@@ -16,5 +40,6 @@
    "predict_batch_transformer",
    "get_device",
    "quantile",
-    "zero_diag"
+    "zero_diag",
+    "TorchDataset"


General point here: Where we have promoted (public!) imports up to a higher submodule (or just moved things around entirely), IMO we at a minimum need to document the changes properly in the release notes. Better yet we should raise DeprecationWarning's if the old imports are used.

An example is the old from alibi_detect.utils.saving import save_detector has been moved to from alibi_detect.saving import save_detector, however calling the former will still import save_detector and raise a DeprecationWarning.

Me and @ascillitoe discussed options:

The best way of providing depreciation warnings is to move the functionality in alibi_detect.utils.pytorch.misc to alibi_detect.utils.pytorch._misc. We then update the __init__.py file to import from _misc.py:

get_device, quantile, zero_diag = import_optional( 'alibi_detect.utils.pytorch._misc', names=['get_device', 'quantile', 'zero_diag'] )

In the misc.py file we replace the relevant functions with a depreciation warning similar to in the config work. I think this makes sense but the result would be inconsistencies in what is private and what is public in a wider sense throughout detect. I could try and make this consistent but this would mean renaming lots of files with the underscore...

IMO moving backend-specific files to private (to avoid imports without optional deps checks) might be a worthwhile exercise in a subsequent PR (to include in 0.10.0 release). However, personally I would only vote for this change if it doesn't screw up the git history for these files too much i.e. if we can get the git history to just track the filename change instead of registering it as a complete deletion and rewrite of each file.

alibi_detect/utils/saving/__init__.py

alibi_detect/utils/tensorflow/__init__.py

ascillitoe · 2022-07-05T08:49:36Z

doc/source/cd/methods/chisquaredrift.ipynb

@@ -112,7 +112,7 @@
 ],
 "metadata": {
  "kernelspec": {
-   "display_name": "Python 3 (ipykernel)",
+   "display_name": "Python 3",


Usual comment wrt to notebooks; ideally would reset these changes to reduce file diffs (same for all following notebooks).

setup.py

ascillitoe

Hey @mauicv, done a first pass. Looking good I think, but will hold off from approving until a few queries are resolved.

ascillitoe · 2022-07-06T14:38:42Z

alibi_detect/models/tensorflow/losses.py

 import tensorflow_probability as tfp
+from tensorflow.keras.losses import kld, categorical_crossentropy


Uber pendantic nitpick, could just remove these changes and changes to test_losses_tf.py...

~~i'm not sure I understand?~~

ascillitoe · 2022-07-06T14:46:52Z

doc/source/cd/methods/spotthediffdrift.ipynb

@@ -123,7 +123,7 @@
    "```python\n",
    "import tensorflow as tf\n",
    "from tensorflow.keras.layers import Conv2D, Flatten, Input\n",
-    "from alibi_detect.utils.tensorflow.kernels import DeepKernel\n",
+    "from alibi_detect.utils.tensorflow import DeepKernel\n",


Change also needs to be made in main text: The DeepKernel class found in either alibi_detect.utils.tensorflow.kernels or alibi_detect.utils.pytorch.kernels

Might be worth grep-ing the docs for all the import statements that have been changed?

Hmm, good idea. I found alibi_detect.utils.pytorch.data.TorchDataset and alibi_detect.utils.tensorflow.data.TFDataset as well, These are referenced in a parameter docstring. Are they intended to be public? Would a user import and use them? They're not used explicitly anywhere within the examples!

In offline discussion agreed the TorchDataset and TFDataset's should be public so I've protected them with import_optional and fixed private import paths in the docs.

* Add BackendValidator class * Protect ad, od and cd and other API objects from tensorflow and torch optional dependency import errors * Update import statements in notebooks

* Implement base optional dependency functionality * Feature Optional backends functionality (#538) * Merge config into opt deps (#553) * Feature Optional prophet (#541) * Feature Optional dependencies documentation updates (#542) * Update licenses (#562) * Add updates to CHANGELOG for optional dependencies (#563)

mauicv added 24 commits May 16, 2022 14:18

Protect ad and od api from optional dependency import errors

1966f2a

Merge branch 'feature/dep-management' into feature/make-tf-optional

739d0a4

Add BackendValidator class

8906e24

Add further Docstrings for BackendValidator and add parameter types

e61e613

Implement tf, tfp and torch dep managment for od, ad, cd, models and …

e2f6a4d

…datasets

Minor fix

aaab509

Protect public api endpoints for tensorflow and torch opt-deps

3c3bc4d

Fix ci bug

449d2ce

Merge branch 'feature/dep-management' into feature/optional-backends-…

9ad2ccb

…functionality

Fix failing tox test

57eb325

Fix backend None issue in model_uncertainty

1303e5e

Fix model_uncertainty issue

544ad05

Update import statments in notebooks 1

0ff03ba

Update import statments in notebooks 2

7f1aaba

Update import statments in notebooks 3

23e125b

Update import statments in notebooks 4

0fe006e

Fix linting error

202e564

Document backend validator tests

cc32e31

Update licenses for new dependencies

e091ae0

Merge branch 'feature/dep-management' into feature/optional-backends-…

c8df9ec

…functionality

Revert large image changes to alibi_detect_deploy.ipynb

9e60f37

Revert licenses change

9be10eb

Remove comment

dbd2ce2

Verify PR temp comments

1b21085

This was referenced Jun 21, 2022

Add optional dependency management #537

Merged

Feature Optional numba #539

Closed

Feature Optional prophet #541

Merged

mauicv added 5 commits June 30, 2022 16:15

Remove tensorflow bucket install from ci

90cb058

Sync feature and optional-backend branch

7dae754

Merge branch 'feature/dep-management' into feature/optional-backends-…

1594926

…functionality

Merge branch 'feature/dep-management' into feature/optional-backends-…

c9e53b2

…functionality

Merge branch 'feature/dep-management' into feature/optional-backends-…

ca51eca

…functionality

ascillitoe reviewed Jul 4, 2022

View reviewed changes

ascillitoe reviewed Jul 5, 2022

View reviewed changes

alibi_detect/utils/tensorflow/__init__.py Outdated Show resolved Hide resolved

ascillitoe reviewed Jul 5, 2022

View reviewed changes

setup.py Show resolved Hide resolved

ascillitoe reviewed Jul 5, 2022

View reviewed changes

mauicv added 4 commits July 5, 2022 15:41

Add elbo loss back into losses file

980d358

Fix tox tests

2feedc9

Refactor _join_url

d632b97

Minor requested changes

09a150a

arnaudvl mentioned this pull request Jul 6, 2022

Add KeOps MMD detector #548

Merged

16 tasks

mauicv added 5 commits July 6, 2022 13:25

Revert unnecessary notebook changes

1d8d220

Remove unnecessary notebook change 2

55736e2

Remove unnecessary notebook changes 3

c384475

Remove unnecessary notebook changes 4

1e37a0f

Remove unnecessary notebook change 5

1d7a6d2

ascillitoe reviewed Jul 6, 2022

View reviewed changes

mauicv added 3 commits July 6, 2022 15:56

Minor fixes

c3fa6d3

Fix minor error

67ae367

Protect TFDataset with import_optional

c49af88

mauicv merged commit 5047c71 into SeldonIO:feature/dep-management Jul 7, 2022

ascillitoe mentioned this pull request Jan 27, 2023

Modular package #388

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature Optional backends functionality #538

Feature Optional backends functionality #538

mauicv commented Jun 21, 2022 •

edited

Loading

review-notebook-app bot commented Jun 21, 2022

ascillitoe commented Jun 28, 2022 •

edited

Loading

mauicv commented Jun 28, 2022

ascillitoe Jul 4, 2022

mauicv Jul 5, 2022

mauicv Jul 6, 2022

ascillitoe Jul 6, 2022

ascillitoe Jul 4, 2022

mauicv Jul 6, 2022

ascillitoe Jul 6, 2022

ascillitoe Jul 5, 2022 •

edited

Loading

ascillitoe left a comment

ascillitoe Jul 6, 2022

mauicv Jul 6, 2022 •

edited

Loading

ascillitoe Jul 6, 2022 •

edited

Loading

mauicv Jul 6, 2022

mauicv Jul 6, 2022

		import tensorflow_probability as tfp
		from tensorflow.keras.losses import kld, categorical_crossentropy

Feature Optional backends functionality #538

Feature Optional backends functionality #538

Conversation

mauicv commented Jun 21, 2022 • edited Loading

Todo:

What is this

1. Make tensorflow and pytorch optional dependencies:

2. Add BackendValidation for all detectors:

What this PR doesn't include:

Notes:

Circular imports:

Changing import statements:

review-notebook-app bot commented Jun 21, 2022

ascillitoe commented Jun 28, 2022 • edited Loading

mauicv commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascillitoe Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

ascillitoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauicv Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

ascillitoe Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauicv commented Jun 21, 2022 •

edited

Loading

1. Make `tensorflow` and `pytorch` optional dependencies:

ascillitoe commented Jun 28, 2022 •

edited

Loading

ascillitoe Jul 5, 2022 •

edited

Loading

mauicv Jul 6, 2022 •

edited

Loading

ascillitoe Jul 6, 2022 •

edited

Loading