Support for serialising detectors with keops backends#681
Support for serialising detectors with keops backends#681ascillitoe merged 9 commits intoSeldonIO:masterfrom ascillitoe:feature/keops_saving_2
Conversation
ascillitoe
added
Type: Serialization
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #681 +/- ##
==========================================
+ Coverage 79.76% 80.16% +0.39%
==========================================
Files 131 133 +2
Lines 9133 9176 +43
==========================================
+ Hits 7285 7356 +71
+ Misses 1848 1820 -28
Flags with carried forward coverage won't be shown. Click here to find out more.
|
jklaise
left a comment
jklaise
left a comment
There was a problem hiding this comment.
A few minor comments but otherwise LGTM!
alibi_detect/utils/keops/kernels.py
Outdated
| cfg = self.config.copy() | ||
| if isinstance(cfg['sigma'], torch.Tensor): | ||
| cfg['sigma'] = cfg['sigma'].detach().cpu().numpy().tolist() |
There was a problem hiding this comment.
Are we certain we don't need a deep copy here (depends on the dict values)? Wouldn't be ideal if self.config turns out to not be idempotent (different depending whether get_config has been called or not.
There was a problem hiding this comment.
I guess the same question applies to every component for which we have self.config and which may have mutable values - do we need to do an audit / have tests?
There was a problem hiding this comment.
Upon review of this I think you're right. We seem to get away with it in our tests (and in the informal tests I did previously) because when we update items in config we are typically redefining the entire item, rather than mutating it (e.g. we change sigma from None to a Tensor, rather than mutating an element in sigma). I think this means the shallow copy is OK since the reference is broken, e.g.:
orig_a = [1,2,3]
dict1 = {'a': deepcopy(orig_a), 'b': 'cat'}
dict2 = dict1.copy()
dict1['a'] = [1,1,1]
dict2['a'] == orig_a
>>> TrueHowever, as you say, if we were to mutate an item in config, we would have a problem:
orig_a = [1,2,3]
dict1 = {'a': deepcopy(orig_a), 'b': 'cat'}
dict2 = dict1.copy()
dict1['a'][1] = 1
dict2['a'] == orig_a
>>> FalseI might be missing something but seems we should add deepcopy to all of these. I can update these ones in this PR and then do a follow-up for the rest of them?
There was a problem hiding this comment.
Yea I think better safe than sorry.
alibi_detect/utils/keops/kernels.py
Outdated
| kernel_a: Union[nn.Module, str] = 'rbf', | ||
| kernel_b: Optional[Union[nn.Module, str]] = 'rbf', |
There was a problem hiding this comment.
I would say to use Literals but I know you'll point me to the other open issue... :)
There was a problem hiding this comment.
Ha ha I hope I'm not developing a reputation for deflecting to other issues!
I can see why Literal might be a good idea here since we really do only accept rbf as a str. Since we'd want to do this for the pytorch and tensorflow kernels it might be best for me to follow-up straight after this PR?
alibi_detect/utils/keops/kernels.py
Outdated
| return similarity | ||
|
|
||
| def get_config(self) -> dict: | ||
| return self.config.copy() |
There was a problem hiding this comment.
Bear with me on this one! We have flavour in ModelConfig etc (see saving.schemas) because we use this to decide which load_model_... function to use (i.e. _tf, _pt or _sk). I've kept flavour decoupled from the detector backend here so that we have the option of using different flavour preprocessing to the model backend (e.g. a scikit-learn preprocessor with a tensorflow MMD detector). Plus, we might have a model even when there is no backend (e.g. any preprocessing model with KS detector).
The situation is a little different for kernels. These are only used for detectors which have backends, and it doesn't make sense to decouple their flavour from backend. Ideally we would not actually have flavour at all for kernels (we actually decide which load_kernel_... function to use based on the detector backend. The only reason we have flavour is so that the coerce_2_tensor pydantic validator knows what type of Tensor to coerce sigma to (this happens before we get to to load_kernel_....
We don't need to do any coercion for DeepKernel hence why no flavour in DeepKernelConfig. I could add it in just for consistency? But it wouldn't actually be used at load time.
alibi_detect/saving/tests/models.py
Outdated
| from alibi_detect.cd.pytorch import HiddenOutput as HiddenOutput_pt | ||
| from alibi_detect.cd.tensorflow import HiddenOutput as HiddenOutput_tf | ||
| from alibi_detect.utils.frameworks import has_keops | ||
| if has_keops: |
There was a problem hiding this comment.
Is this conditional purely because we skip keops on some platforms in CI? Should we add a comment for this? Same question for the other test module.
There was a problem hiding this comment.
Yep precisely. I'll add notes to both modules now!
Edit: Done in 5bb7314
|
@jklaise would you be able to look into the responses to your comments when you have a chance, please? |
4 participants
Following on from #656, this PR adds support for serialising detectors with
backend='keops'. Since keops detectors are still pytorch based, this is a relatively simple addition. The main changes involve removingNotImplementedError's and support for saving keops kernels.