[GSK-1684] removing validation flags for model evaluation

[rabah-khalek]

moving saving and loading validation to model.upload(), removing ValidationFlags

[linear]

GSK-1684 removing `ValidationFlags`

mattbit : the argument default value is set to a mutable, which causes potentially undesired side effects (Python evaluates args upon function definition, not execution). I think we need much more careful in writing things in a modular way and avoid entangling objects as much as possible. In particular when it comes to the public API like in this case. The Giskard library is already jam-packed with bugs and if we are not careful we will just make it completely unusable.

Two ideas:

• You introduced all of this because you need to disable the model_loading_and_saving check, I am convinced that we should remove this check by default and only execute it (separately) when people try to upload to the giskard hub. In this way, people who have some complex non-serializable model can still use the programmatic API and at least run the scan. If they can run scan, setup test suites, etc. they will be more motivated in refactoring their implementation later to make it compatible with the giskard hub.
• For situations like the one you tried to solve I would prefer a context manager, since it can act globally (passing a flags object feels very C++).

It would be something like

with models.validation(load_and_save="ignore"):
    do_what_I_need_to_do(model)

This is what is used for example in numpy (np.errstate context manager). If you want a super simple implementation example, have a look at  giskard/models/cache/__init__.py: there is a no_cache context manager which is used to temporarily disable the validation cache. But up to you — I would just remove the load&save check completely and add it before the upload only.

[mattbit]

I left a small comment but looks good to me!

[mattbit]

nitpick: I would first run validate_model which checks a lot of formal things, then try loading_and_saving. I would stick to the original model, not another object.

[rabah-khalek]

I particularly written it that way (which was also the previous logic) because we also want to check that the reloaded model works. Ideally we would do:

validate_model(model=self, validate_ds=validate_ds)
reloaded_model = validate_model_loading_and_saving(self)
validate_model(model=reloaded_model, validate_ds=validate_ds)

WDYT?

[andreybavt]

For us it's important to know that the deserialized model works. So I think we should validate the loaded one and if it fails we could also run validation on the original model to give a hint that it might be a problem with the serialization

[rabah-khalek]

in fact the validation of the original model already happens in the scan, but it doesn't hurt running it in upload() too (just in case a user bypassed the scan).

So what you're suggesting @andreybavt is something like:

reloaded_model = validate_model_loading_and_saving(self)
try:
  validate_model(model=reloaded_model, validate_ds=validate_ds)
except Exception as e_reloaded:
  try:
    validate_model(model=self, validate_ds=validate_ds)
  except Exception as e_loaded:
    raise e_loaded
  raise GiskardException("An error occured during the deserialisation of your model, please report this issue to giskard, etc.") from e_reloaded

[andreybavt]

almost, we just need to make sure we don't lose the e_reloaded when you throw e_loaded

so something like

reloaded_model = validate_model_loading_and_saving(self)
try:
    validate_model(model=reloaded_model, validate_ds=validate_ds)
except Exception as e_reloaded:
    try:
        validate_model(model=self, validate_ds=validate_ds)
        logger.info("Original model validated successfully")
    except Exception as e_loaded:
        logger.exception("Failed to validate on original model", e)
    raise GiskardException(
        "An error occured while validating a deserialized version your model, please report this issue to Giskard") from e_reloaded

[rabah-khalek]

ok done

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

94.1% Coverage
0.0% Duplication