-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
GSK-146: update demo projects with new giskard client #89
Conversation
@@ -20,6 +20,9 @@ scikit-learn = ">=1.0.2" | |||
shap = ">=0.40.0" | |||
zstandard = "^0.15.2" | |||
numpy = "^1.21.6" | |||
#torch = ">=1.10.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason to have them commented?
try { | ||
saveProject(key, config.creator); | ||
} catch (IOException e) { | ||
logger.error(e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two logging calls looks redundant, the second one will include e.getMessage() too
giskard-server/src/main/java/ai/giskard/service/InitService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the PR looks good to me now. I tested the demo models and datasets, it works fine.
My two last comments are:
- model names (see
uploadModel
) - German_credit_scoring_giskard.ipynb is still using ModelInspector, can you adapt it to the new API? It's the notebook that we ship with our demo
jupyter
container.
Resource modelResource = resourceLoader.getResource(pathToModel); | ||
Resource requirementsResource = resourceLoader.getResource(pathToRequirements); | ||
ModelUploadParamsDTO modelDTO = projects.get(projectKey).modelParams; | ||
modelDTO.setName(modelDTO.getName() + " " + filename); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're modifying the shared ModelUploadParamsDTO
. This results in model names like "German credit score 0 1" because on the first iteration "0" was already appended to the name. We should create a copy of projects.get(projectKey).modelParams
here before modifying it.
Kudos, SonarCloud Quality Gate passed! |
No description provided.