[GSK-1343] Better support for CUDA and models + docs improvements

[mattbit]

• Added support for batching in WrapperModel
• Fixed a bug in CatboostModel that prevented catboost models from being loaded correctly
• Reorganized the models package and revised all docstrings
• Added tests for CatboostModel, WrapperModel, BaseModel, CloudpickleSerializableModel
• Re-enabled skipped tests that now can work! (test_catboost_changed_column_order and test_tabular_titanic_binary_classification)
• Added all model wrappers to the docs, under “API Reference”

TODO

• write test for batching
• manual test with pytorch on CUDA
• manual test with tensorflow on CUDA

[linear]

GSK-1343 Bug running HF model on GPU

When running model predictions, we do:

logits = predictions.logits.detach().numpy()

but we should first move the tensors to CPU or it will fail with

TypeError: can't convert cuda:0 device type tensor to numpy. Use Tensor.cpu() to copy the tensor to host memory first.

[andreybavt]

@mattbit

1. Sure, covering download with tests is a good idea
2. In case you want to create a custom model you'd inherit a CustomModel class in this case you'd use a load from BaseModel

[rabah-khalek]

• the batch_size to be saved and reloaded properly in WrapperModel
• good catch the catboost missing load_model, I remember vividly to have fixed it, but maybe in a branch that has never been merged.
• good job on the tests
• good job on the doc
• Overall I like the new refactoring of models, the previous modular structure we followed only makes sense if we didn't have classes, only collection of methods (such as mlflow)

[rabah-khalek]

Could we rewrite this in a way that shows these 4 classes as a drop-down list when we click on "Base model classes" on the left toc menu?

[mattbit]

You mean the sidebar?

[rabah-khalek]

yes, the left sidebar - not really urgent this one. I admittedly tried to do it myself, wasn't super intuitive without some files re-organization.

[rabah-khalek]

A not-so-urgent suggestion: to avoid devs from forgetting the saving of daughter model class' meta such as batch_size in your case, would be wise to extend model_validation in https://github.com/Giskard-AI/giskard/blob/c860a43a7ba1b7a5c6e11ddeafe7047d9f82409a/python-client/giskard/core/model_validation.py#L126 to compare the attributes values (as much as possible) before and after the saving, with an associated test

[mattbit]

A not-so-urgent suggestion: to avoid devs from forgetting the saving of daughter model class' meta such as batch_size in your case, would be wise to extend model_validation in

https://github.com/Giskard-AI/giskard/blob/c860a43a7ba1b7a5c6e11ddeafe7047d9f82409a/python-client/giskard/core/model_validation.py#L126
to compare the attributes values (as much as possible) before and after the saving, with an associated test

The problem with that would be deciding which attributes should be checked. I’m not sure that all model attributes would need to be persisted and reloaded.

[rabah-khalek]

hmm... Why not? which attributes you have in mind?
I think outside kwargs that are passed externally upon loading (and are not attributes anyways):

class BaseModel(ABC):
    @configured_validate_arguments
    def __init__(
        self,
        model_type: ModelType,
        name: Optional[str] = None,
        feature_names: Optional[Iterable] = None,
        classification_threshold: Optional[float] = 0.5,
        classification_labels: Optional[Iterable] = None,
        **kwargs,
    ) :
    ...

    @classmethod
    def load(cls, local_dir, **kwargs):
    ...

all attributes should be checked no?

[rabah-khalek]

Thanks for the update @mattbit, I just did some minor refactoring in a70b89a. To me, it's good to go.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
9 Code Smells

87.8% Coverage
0.0% Duplication