feat: add stable diffusion rediffuse artifact probe#257
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a new artifact probe for auditing collaborator-provided Stable Diffusion ReDiffuse results, adding the probe-rediffuse-sd-artifacts CLI command and associated logic in rediffuse_sd.py. The changes also include comprehensive updates to the research roadmap, workspace documentation, and unit tests. Review feedback highlights the need for improved error handling in the probe, specifically suggesting the inclusion of OSError in exception blocks and defensive checks for null values in the metrics JSON to prevent potential runtime crashes.
| metrics_payload = _read_json(root / "metrics.json") | ||
| rows, labels, scores, predictions, correct = _read_result_csv(root / "result.csv") | ||
| fpr, tpr = _read_curve_csv(root / "roc_curve.csv") | ||
| except (ValueError, TypeError, KeyError, json.JSONDecodeError) as exc: |
There was a problem hiding this comment.
The try...except block should also catch OSError to handle cases where a required path exists but is inaccessible (e.g., due to permission issues) or is a directory instead of a file. Currently, path.read_text() or path.open() would raise an exception that crashes the CLI in these scenarios instead of returning a blocked status payload.
| except (ValueError, TypeError, KeyError, json.JSONDecodeError) as exc: | |
| except (ValueError, TypeError, KeyError, json.JSONDecodeError, OSError) as exc: |
| reported_auc = float(metrics_payload.get("auc", float("nan"))) | ||
| reported_asr = float(metrics_payload.get("asr", float("nan"))) | ||
| reported_tpr_1pct = float(metrics_payload.get("tpr_at_fpr_1pct", float("nan"))) | ||
| reported_member_count = int(metrics_payload.get("num_member", -1)) | ||
| reported_nonmember_count = int(metrics_payload.get("num_nonmember", -1)) |
There was a problem hiding this comment.
The extraction of reported metrics from metrics_payload is not defensive against null values in the JSON file. If a key like "auc" is present but its value is null, metrics_payload.get("auc") will return None, causing float(None) to raise a TypeError and crash the CLI. Since this block is outside of a try...except handler, it should handle None values explicitly.
| reported_auc = float(metrics_payload.get("auc", float("nan"))) | |
| reported_asr = float(metrics_payload.get("asr", float("nan"))) | |
| reported_tpr_1pct = float(metrics_payload.get("tpr_at_fpr_1pct", float("nan"))) | |
| reported_member_count = int(metrics_payload.get("num_member", -1)) | |
| reported_nonmember_count = int(metrics_payload.get("num_nonmember", -1)) | |
| reported_auc = float(metrics_payload.get("auc") if metrics_payload.get("auc") is not None else "nan") | |
| reported_asr = float(metrics_payload.get("asr") if metrics_payload.get("asr") is not None else "nan") | |
| reported_tpr_1pct = float(metrics_payload.get("tpr_at_fpr_1pct") if metrics_payload.get("tpr_at_fpr_1pct") is not None else "nan") | |
| reported_member_count = int(metrics_payload.get("num_member") if metrics_payload.get("num_member") is not None else -1) | |
| reported_nonmember_count = int(metrics_payload.get("num_nonmember") if metrics_payload.get("num_nonmember") is not None else -1) |
Summary
Verification