Safety Suite Update#208
Conversation
undfined
left a comment
undfined
left a comment
There was a problem hiding this comment.
Looks good overall! I few things we should address before merging.
| judge_response["raw_judge_response"] = response | ||
| logger.debug("Judge response: %s", judge_response) | ||
| instance.metadata["judge_result"] = judge_response | ||
| instance.metadata["is_parsing_error"] = ( |
There was a problem hiding this comment.
Can we clarify the intended behavior for partial judge parse errors?
Right now is_parsing_error is set when any of the three judge fields is invalid, but the final score can still be 1.0 if the specific target field matches the gold answer. For example, WildGuard output with prompt_harmfulness=maybe and response_harmfulness=no can score correct while is_parsing_error=True.
If the intended policy is strict parsing, this should return 0.0 whenever is_parsing_error is true. If the intended policy is to allow partial parses, then we should rename/document this clearly and only treat the target field’s invalid value as score-blocking. Either way, we should add an explicit test so the scoring and parsing_error metric stay consistent.
| answer_extractor = extract_think_answer | ||
|
|
||
| @property | ||
| def instances(self) -> Iterator[Instance]: |
There was a problem hiding this comment.
This task repeats the same instances, request_type, and format_request logic used in the other safety task adapters. Could we factor this into a shared safety task base class or helper? That would make these adapters easier to maintain and reduce the chance that future fixes only get applied to some files.
| ) | ||
|
|
||
|
|
||
| def safety_metrics(scorer, subsets): |
There was a problem hiding this comment.
This helper is used by task modules, but it lives in a suite module. That makes the dependency direction a bit awkward because tasks need to import from evals.suites.safety just to define their metrics. Could we move safety_metrics to a task/common or metrics-oriented module instead?
| def process_doc(self, doc: dict[str, Any], index: int = 0) -> Instance | None: | ||
| """Convert a dataset document to an Instance.""" | ||
|
|
||
| if doc["data_type"] == "adversarial_benign": |
There was a problem hiding this comment.
Can we add an explicit else here for unexpected data_type values? As written, if the dataset contains anything other than adversarial_benign or adversarial_harmful, prompt_type and gold_answer will be referenced before assignment. Returning None or raising a clear error would make this safer.
There was a problem hiding this comment.
judge_format looks like it only supports a fixed set of values, currently "standard" and "wildguard". Could we type this as Literal["standard", "wildguard"] or validate it in __post_init__? That would catch typos earlier and make the supported modes clearer.
| parsed_result = parsed_result.replace("2. ", "", 1) | ||
| q2_answer, parsed_result = parsed_result.split("\n", 1) | ||
| q2_answer = q2_answer.strip().lower() | ||
| q2_answer = answer_map[q2_answer] |
There was a problem hiding this comment.
This parsing path can raise KeyError for unexpected Q2 values because answer_map[q2_answer] is only protected by except ValueError. For example, judge output like refusal instead of refuse will escape this block before is_parsing_error is written.
Could we catch both malformed structure and invalid answer labels here, set judge_result, set is_parsing_error=True, and return 0.0? That keeps parse failures from turning into a later metric crash.
| if not responses: | ||
| return 0.0 | ||
|
|
||
| total = sum(int(r.instance.metadata["is_parsing_error"]) for r in responses) |
There was a problem hiding this comment.
This metric assumes every response has instance.metadata["is_parsing_error"], but that is only true if the safety scorer reached the metadata-writing path. If judge parsing or provider scoring raises before that, this metric can crash the task.
Could we make this defensive, e.g. use .get("is_parsing_error", False), or ensure the scorer always initializes this metadata before parsing?
| for r in responses: | ||
| if subset == "any" or r.instance.metadata.get(subset) == cat: | ||
| subset_accuracy.append(r.scores.get(scorer_name)) | ||
| if r.instance.metadata.get("context_condition") == metric or metric == "accuracy": | ||
| if r.instance.metadata.get("bias") is not None: | ||
| subset_bias.append(r.instance.metadata.get("bias")) | ||
| if r.instance.metadata.get("nonunknown") is not None: | ||
| subset_nonunknown.append(r.instance.metadata.get("nonunknown")) |
There was a problem hiding this comment.
This helper appears to mix denominator scopes. subset_accuracy includes every selected response, but when computing ambig / disambig, subset_bias and subset_nonunknown only include responses matching that context condition. That means the final bias metrics can use an all-context accuracy with a filtered-context bias score. Is that the intended behavior?
| logprob_sums = [scorer.score(r.instance, o) for o in r.outputs] | ||
| correct.append(logprob_sums.index(max(logprob_sums)) != gold_idx) | ||
|
|
||
| return sum(correct) / len(correct) |
There was a problem hiding this comment.
This can divide by zero when no responses match the requested subset. That is reachable in small limit smoke runs because limits randomly sample instances, so a subset like wmdp-chem may have no examples.
Could we return a sentinel value like -1 or 0.0 when correct is empty, matching the behavior used by other subset metrics?
|
|
||
|
|
||
| @dataclass(frozen=True, slots=True) | ||
| class SafetyErrorMetric(Metric): |
There was a problem hiding this comment.
SafetyErrorMetric should override compute_instance. Right now it inherits the default Metric.compute_instance, which falls back to the scorer value when there is no response.scores["parsing_error"]. For this metric, that means the per-instance parsing_error value can become the safety score instead of the parse-error flag.
For example, if metadata["is_parsing_error"] is True and the safety scorer returned 0.0, the aggregate metric counts the parse error as 1, but compute_instance() returns 0.0.
Could we implement compute_instance here to return something like float(response.instance.metadata.get("is_parsing_error", False))? Let's add a simple unit test for this since it feels a bit complex.
…gation' into maliam/base_safety_eval
2 participants
Description
Add from the olmo3 safety suite: Do_Anything_Now, Wildguard, Wildjailbreak, BBQ, WMDP, TrustLLM JailbreakTrigger, and XSTest.
Add a base version of each eval.
Add a safety suite.
Fix the wildguard judge length maximum restriction
Type of Change
Testing
pytest tests/ --ignore=tests/integration/)Checklist