-
-
Notifications
You must be signed in to change notification settings - Fork 377
GSK-2902 GSK-2418 Add a method to execute test suite and upload result to Hub programmatically + result cleanup #1818
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
Conversation
giskard/client/dtos.py
Outdated
|
|
||
|
|
||
| class TestSuiteExecutionResult(str, Enum): | ||
| IN_PROGRESS = ("IN_PROGRESS",) |
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.
Why ( "", ), and "ERROR" ? are the parenthesis needed ?
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.
My bad I added unnecessary commas and the pre-commit reformatted like that
| metadata: Dict[str, List[str]] | ||
|
|
||
|
|
||
| class SaveSuiteTestExecutionDTO(ConfiguredBaseModel): |
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.
That's a lot of data. Everything is needed ?
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.
Everything is saved into the Hub.
I agree that we don't use nor need all those information into the Hub. We might probably want to cleanup everything. (Both from Hub and programatic execution)
giskard/core/suite.py
Outdated
| def _test_result_to_dto(result: SuiteResult): | ||
| datasets = {dataset.id: dataset for dataset in result.params.values() if isinstance(dataset, Dataset)} | ||
|
|
||
| return SaveSuiteTestExecutionDTO( |
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.
How do we have so many field 😱
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.
Created issue and listed unused stuff: https://linear.app/giskard/issue/GSK-2902/cleanup-testresult-information-that-are-sent-to-the-hub
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.
I can work work on this on this PR it's relatively safe to clean this up
# Conflicts: # giskard/core/suite.py
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.
I did give this PR a try, and had some issues :
- I had some issues with suite_id not being set (I added a commit for this)
- I cannot debug uploaded test results (issue with uuid badly formatted ?
- when executing test suite from downloaded one, the labels are different (maybe unrelated)
|
Execution in notebook uploaded (see with uuid) Finally download + execution in notebook + uploade (see label of tests) The notebook |
|
@Hartorn Thanks for the feedback, I fixed the issue with the |
|
@kevinmessiaen I did try it again, but still cannot debug |
Yes it was the wrong id send ( I updated the save so that we can keep track of all generated ids properly |
b93076a to
c7eabb1
Compare
|






Description
Added an
uploadmethod to theTestResultin order to allow saving them to the Hub.The method have the following params:
Nonethe label of the execution. IfNonethe execution date will be the labelExample code to execute:
Cleanup of test result information sent to up
Removed unused inforamtion:
Related Issue
Type of Change