Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Predictor attr not properly cleared when saving #930

Open
ascillitoe opened this issue Jun 13, 2023 · 2 comments
Open

Predictor attr not properly cleared when saving #930

ascillitoe opened this issue Jun 13, 2023 · 2 comments
Labels
Type: Bug Something isn't working Type: Serialization Serialization proposals and changes

Comments

@ascillitoe
Copy link
Contributor

ascillitoe commented Jun 13, 2023

When saving explainers, the predictor attribute is set to None to avoid attempting to save it, and it is then given to load_explainer at load time:

alibi/alibi/saving.py

Lines 110 to 112 in c2da012

def _simple_save(explainer: 'Explainer', path: Union[str, os.PathLike]) -> None:
predictor = explainer.predictor # type: ignore[attr-defined] # TODO: declare this in the Explainer interface
explainer.predictor = None # type: ignore[attr-defined]

However, some explainers, such as AnchorTabular, set other protected attributes just as _predictor when setting the predictor property:

@predictor.setter
def predictor(self, predictor: Optional[Callable]) -> None: # Optional here because in saving.py we set it to None
# deal with the case from saving.py first
# TODO: how do we prevent users from passing predictor=None? Probably beartype.
if predictor is None:
self._predictor = None
if self.ohe:
self._ohe_predictor = None
else:
# if input is one-hot encoded
if self.ohe:
# this predictor expects ordinal/labels encoded categorical variables
ord_predictor = lambda x: predictor(ord_to_ohe(x, self.cat_vars_ord)[0]) # noqa: E731
self._predictor = self._transform_predictor(ord_predictor)
# this predictor expects one-hot encoded categorical variable
self._ohe_predictor = self._transform_ohe_predictor(predictor)
else:
# set the predictor
self._predictor = self._transform_predictor(predictor)

These protected attributes are not cleared when saving, and are included in the explainer.dill file. I'm don't have a good example of where this is actually a problem at load time, but we should investigate, and if needed implement a more robust approach to clearing self.predictor. This might be as simple as something like:

@predictor.deleter
def predictor(self):
	del self._predictor
        if self.ohe:
             del self._ohe_predictor

and then del explainer.predictor in _simple_save.

@ascillitoe ascillitoe added the Type: Serialization Serialization proposals and changes label Jun 13, 2023
@jklaise jklaise added the Type: Bug Something isn't working label Jun 13, 2023
@jklaise
Copy link
Contributor

jklaise commented Jun 13, 2023

Related issue: #830

We probably shouldn't straight-up delete or set attributes to None as when serialization fails then the explainer is left in a broken state. Instead, the attributes should be cleared whilst maintaining a local variable with a reference to the object and if for some reason serialization fails we can reset the references to restore the explainer to its state pre-calling .save.

@ascillitoe
Copy link
Contributor Author

Good point, didn't think about that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working Type: Serialization Serialization proposals and changes
Projects
None yet
Development

No branches or pull requests

2 participants