Fix to allow config.toml to be loaded with [meta] not present #591
Conversation
Codecov Report
@@ Coverage Diff @@
## master #591 +/- ##
==========================================
- Coverage 84.77% 84.74% -0.04%
==========================================
Files 202 202
Lines 13538 13528 -10
==========================================
- Hits 11477 11464 -13
- Misses 2061 2064 +3
|
| @@ -259,6 +268,22 @@ def preprocess_hiddenoutput(classifier, backend): | |||
| return preprocess_fn | |||
|
|
|||
|
|
|||
| @parametrize('cfg', CFGS) | |||
| def test_load_simple_config(cfg, tmp_path): | |||
There was a problem hiding this comment.
Worth adding a docstring saying exactly what this is testing, i.e. not using the save_detector functionality here as that adds [meta] field automatically?
| # Save config.toml then load it | ||
| with open(cfg_path, 'w') as f: | ||
| toml.dump(cfg, f) | ||
| cd = load_detector(cfg_path) |
There was a problem hiding this comment.
Is it necessary to dump to .toml first instead of calling load_detector on the dictionary? I suppose if we're strictly testing loading of .toml then it's ok.
There was a problem hiding this comment.
I agree with you on this one, being able to pass config dicts to load_detector is something I think is nice. However, I can't recall why now but sometime back we decided to remove this capability (I think just to reduce complexity). The function signature for load_detector is currently load_detector(filepath: Union[str, os.PathLike], **kwargs).
I'd quite like to revisit this at some point...
| with open(cfg_path, 'w') as f: | ||
| toml.dump(cfg, f) | ||
| cd = load_detector(cfg_path) | ||
| assert cd.__class__.__name__ == cfg['name'] |
There was a problem hiding this comment.
Is there a way to assert "more", e.g. get the config dict?
alibi_detect/saving/validate.py
Outdated
| meta = cfg.get('meta') | ||
| meta = {} if meta is None else meta |
There was a problem hiding this comment.
Why not meta = cfg.get('meta', {})? Ease of reading?
There was a problem hiding this comment.
This bit was the root of the bug. The pydantic schema for DetectorConfig sets meta to None if it doesn't exist in the config.toml. Since meta does exist in cfg (but equals None), cfg.get('meta', {}) will still return None, and then line 42 fails.
There was a problem hiding this comment.
Is this something worth leaving a comment in the code for (to prevent overzealous code "clean-up")?
There was a problem hiding this comment.
Good idea. Added a comment to this and the line in base.py.
alibi_detect/base.py
Outdated
| meta = config.pop('meta', None) | ||
| meta = {} if meta is None else meta |
There was a problem hiding this comment.
Why not meta = config.pop('meta', {})? Ease of reading?
There was a problem hiding this comment.
See previous comment.
|
|
||
| # Define a detector config dict without meta (as simple as it gets!) | ||
| mmd_cfg_nometa = mmd_cfg.copy() |
There was a problem hiding this comment.
Do we need to worry about shallow copies and shared objects here?
There was a problem hiding this comment.
I did check and it doesn't seem to be an issue. Which is confusing since I think it would be for a list i.e. the pop-ing some from mmd_cfg_nometa would have removed it from mmd_cfg too 🤷🏻♂️
I've changed it to deepcopy anyway just to be on the safe side...
#564 introduced a bug whereby
config.tomlfiles were not loadable viaload_detectorif the[meta]field was not present. The[meta]field is only present whenconfig.tomlfiles are generated bysave_detector, hence why this was not picked up during previous CI.This PR fixes this issue, and adds tests to check that
validate_configandload_detectorboth work when[meta]is not present.This will be released in a
v0.10.3patch once merged.