Skip to content

feat(reporting): add legacy report.json conversion script#457

Open
ssrhaso wants to merge 10 commits into
mainfrom
456-report-conversion-script
Open

feat(reporting): add legacy report.json conversion script#457
ssrhaso wants to merge 10 commits into
mainfrom
456-report-conversion-script

Conversation

@ssrhaso

@ssrhaso ssrhaso commented May 20, 2026

Copy link
Copy Markdown
Contributor

closes #456

summary

implements the legacy report.json conversion script requested in issue #456. legacy flat reports are upgraded to the new nested, catalog-enriched format and validated against the bundled json schema, entirely offline, so existing outputs can feed the new reporting pipeline without re-running any attacks.

what it does

the four steps listed on the issue are all covered. top-level experiments are wrapped under "attacks". the four catalogs (metric_catalog, parameter_catalog, attack_category_catalog, attack_catalog) are injected from a bundled, editable
catalog_definitions.json. the result is validated against the draft-07 schema with jsonschema, and any uncatalogued metric, parameter, attack, or attack category is reported as a non-fatal warning.

legacy metadata is also normalised so it actually satisfies the schema (provided in the issue), covering missing sacroml_version, instance-less structural attacks, non-scalar global_metrics, and instance keys that do not match instance_<n>. curve arrays (fpr, tpr, roc_thresh) are passed through untouched, with the known roc_thresh null-prefix gap surfaced as a notice rather than an error.

interface

a new sacroml.reporting subpackage exposes convert_report, convert_report_file, and ConversionResult. a matching cli command, sacroml convert-report <input> <output> [--no-validate], wraps the same entry point and is documented in docs/source/attacks/report.rst. jsonschema is added as a runtime dependency, and only the schema and catalog files are bundled as package data (explicit list, no wildcard).

validation

ruff and ruff format pass on the new code, and all 19 tests in tests/reporting/ pass. an 11 mb real-world example report from the issue thread was confirmed schema-valid by this converter during development and can be recreated by downloading the legacy
report.json attached to issue #456 and running:

sacroml convert-report report.json attack_report_example_new.json

ssrhaso added 2 commits May 19, 2026 12:19
Add `sacroml convert-report` CLI plus a `sacroml.reporting` subpackage
that converts legacy flat report.json files into the new nested,
catalog-enriched format and validates them against the bundled
JSON schema. Includes catalog definitions, schema, tests, and docs.
Add `sacroml convert-report` CLI plus a `sacroml.reporting` subpackage
that converts legacy flat report.json files into the new nested,
catalog-enriched format and validates them against the bundled
JSON schema. Includes catalog definitions, schema, tests, and docs.
@ssrhaso ssrhaso requested a review from JessUWE May 20, 2026 10:05
@ssrhaso ssrhaso changed the title 456 report conversion script feat(reporting): add legacy report.json conversion script May 20, 2026
@ssrhaso ssrhaso self-assigned this May 20, 2026
@ssrhaso

ssrhaso commented May 20, 2026

Copy link
Copy Markdown
Contributor Author

#456 @JessUWE

@ssrhaso ssrhaso requested a review from jim-smith May 20, 2026 10:12
@codecov

codecov Bot commented May 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.66%. Comparing base (483b641) to head (d68e476).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #457      +/-   ##
==========================================
+ Coverage   99.65%   99.66%   +0.01%     
==========================================
  Files          27       29       +2     
  Lines        3436     3577     +141     
==========================================
+ Hits         3424     3565     +141     
  Misses         12       12              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ssrhaso added 2 commits May 20, 2026 11:46
Add tests that drive every remaining defensive isinstance/fallback path
in sacroml.reporting.convert: non-dict global_metrics, attack_params,
target_model_params, target_train_params and attack_experiment_logger;
non-string target_model and log_time; experiment keys with illegal
characters; non-dict attack_instance_logger; instance-key collision
during renaming; an already-wrapped report whose experiment value is
not a dict; a non-curve schema error in an instance metric; and the
short/unrelated/missing-lookup branches of _is_curve_violation.
@ssrhaso

ssrhaso commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

did a small tidy up on top, no behaviour change:

  • new ConversionResult.summary() and summary_dict() so callers get the same text/counts the cli prints.
  • main.py cli collapsed to one print(result.summary(...)) call.
  • _normalise_instance_logger now packs renamed keys into compact instance_0/1/2 slots.
  • _diff is now a single-pass partition.
  • 5 new tests, 35/35 reporting tests pass, ruff clean.

@ssrhaso

ssrhaso commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

@JessUWE

@rpreen

rpreen commented May 28, 2026

Copy link
Copy Markdown
Contributor

This wasn't my issue, but looking at the diff it slightly concerns me that the original issue seems to ask for a simple script, yet this introduces quite a few new modules and tests - for how long will we be maintaining and supporting all of this just for this legacy script conversion?

… core

Address review feedback that the tooling outgrew the issue. Remove speculative normalisation and the post-hoc ConversionResult.summary/summary_dict API, keeping only what R1-R4 and the real example reports require: wrap, catalog injection, schema validation, uncatalogued warnings, sacroml_version injection and the curve-violation downgrade. convert.py 570->377, tests 40->20, legacy_edge fixture removed.
@ssrhaso

ssrhaso commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

kept what the issue needs (wrap, catalogs, validate, warn) plus the two things real reports need to validate (sacroml_version injection, roc_thresh/curve downgrade). cut the speculative normalisation and the summary/summary_dict api that only a hand-built fixture exercised; malformed data now surfaces as a schema error instead of being silently rewritten.

net: convert.py 570 to 377, tests 40 to 20, legacy_edge fixture gone (~460 fewer lines). both real reports still convert and validate.

@ssrhaso

ssrhaso commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

This wasn't my issue, but looking at the diff it slightly concerns me that the original issue seems to ask for a simple script, yet this introduces quite a few new modules and tests - for how long will we be maintaining and supporting all of this just for this legacy script conversion?

@rpreen Made some changes

@ssrhaso ssrhaso requested a review from rpreen May 29, 2026 15:01
@jim-smith

Copy link
Copy Markdown
Contributor

@hannah-gordon-nss does this do what you want/need ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[New Feature Request] Create a conversion script to reformat existing report.json files into the new format for reporting

3 participants