-
-
Notifications
You must be signed in to change notification settings - Fork 248
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
JSON report generation from scan results #1929
Conversation
@bmalezieux you might need something different, but for reference the AVID integration could be helpful (it also generates JSON reports from the scan, following the AVID schema). |
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.
since <detector>.run()
produces a list of issues, which then you're looping over to jsonify, I would suggest to implicitly infer the detector names inside the run, and add this info to the issue's meta, which then you can group based on in to_json
of ScanReport
. detectors_names
shouldn't be an input to the report, as we already have this info somewhere.
giskard/core/suite.py
Outdated
def to_json(self, filename=None): | ||
results = {} | ||
for suite_result in self.results: | ||
results[suite_result.test_name] = "Passed" if suite_result.result.passed else "Failed" |
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.
can we add all the meta data that is available / could be useful?
- metric value
- model name
- dataset(s) name(s)
giskard/core/suite.py
Outdated
with open(filename, "w") as json_file: | ||
json.dump(results, json_file, indent=4) | ||
else: | ||
return results |
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 would perhaps return the json representation instead of the dict here
return json.dumps(results, indent=4)
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.
Or let's simply remove this else, and always return the results
giskard/core/suite.py
Outdated
@@ -574,6 +585,7 @@ def run(self, verbose: bool = True, **suite_run_args): | |||
|
|||
if isinstance(result, bool): | |||
result = TestResult(passed=result) | |||
print("TEMP: ", test_partial.test_id, result.passed) |
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.
to be removed
@@ -18,7 +19,7 @@ | |||
|
|||
|
|||
class ScanReport: | |||
def __init__(self, issues, model=None, dataset=None, as_html: bool = True): | |||
def __init__(self, issues, model=None, dataset=None, detectors_names=None, as_html: bool = True): |
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 thought the idea was to remove this argument. Do we really need to know which detector didn't produce issues?
@@ -51,3 +51,25 @@ def test_scan_report_exports_to_markdown(): | |||
assert dest.exists() | |||
assert dest.is_file() | |||
assert dest.read_text() == markdown | |||
|
|||
|
|||
def test_scan_report_to_json(): |
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.
let's add a test where you pass the detector-names list to the scanreport, and check if the detector is in the json file
Please retry analysis of this Pull-Request directly on SonarCloud |
Description
Related Issue
Type of Change
Checklist
CODE_OF_CONDUCT.md
document.CONTRIBUTING.md
guide.pdm.lock
runningpdm update-lock
(only applicable whenpyproject.toml
has beenmodified)