FIX: reject a SeedDatasetFilter axis given an empty set

[Chen Yufeiyang (feiiiiii5)]

Description

SeedDatasetFilter reads None on a filter axis as "this axis is not requested", but an empty set is something else, and the two matching modes read it in opposite ways: without strict_match nothing can overlap with set(), so an empty axis matches no dataset; with strict_match nothing can be outside set(), so the same axis matches every dataset. One flag decides whether a filter returns nothing or the whole catalogue, with no error either way.

SeedDatasetFilter._validate already rejects the other logically impossible strict_match configuration at construction time (size={"small", "large"}), so this one belongs there too:

SeedDatasetFilter(harm_categories=set(user_categories))  # user_categories == []

now raises ValueError naming the axis and telling the caller to pass None to leave it unfiltered. None keeps its meaning, and the matching code is unchanged.

If you would rather have an empty set mean "no constraint on this axis" — the other reading, and a one-liner in _match_single_criterion — say so and I will switch it; I went with the error because both current readings surprise, and a red-teaming run that silently loads the whole catalogue is the expensive one.

Tests and Documentation

Four cases in tests/unit/datasets/test_seed_dataset_metadata.py: an empty set is rejected for a flat filter, under strict_match, and inside criteria=[] (the first three fail on main with DID NOT RAISE), and None is still accepted as "axis not requested".

pytest tests/unit/datasets/ -q gives 4874 passed, 1 failed; the failure is test_local_prompt_dataset_semantics.py::test_airt_fairness_builds_coherent_attack_parameters, which fails the same way with this change reverted (PackageNotFoundError from importlib.metadata). ruff check and ruff format --check are clean on both files.

[Chen Yufeiyang (feiiiiii5)]

Good catch, and you are right that there is no ambiguous matching to protect against here — the caller asked for every dataset, so an empty axis beside all is a field being ignored, not a filter that matches nothing.

Confirmed the mechanism before changing it: the empty-axis check sat above if not self.has_all_tag: return, so the override never got a chance.

$ SeedDatasetFilter(tags={"all"}, harm_categories=set())
ValueError: Filter axes ['harm_categories'] were given an empty set, ...

Done in ba3ddbee. The check is now skipped when any criterion carries all, and the existing warnings below still run — so combining all with other fields is still reported, which is what you asked to keep.

Tests for both construction paths you named:

• test_all_tag_keeps_an_empty_axis_accepted — the single-criterion form above
• test_all_tag_in_one_criterion_covers_an_empty_axis_in_another — composed criteria with one all and one empty axis, parametrized over both strict_match values

pytest tests/unit/datasets/test_seed_dataset_metadata.py -q    54 passed
ruff check / ruff format --check on both changed files        clean

All three new cases fail on 9934c6dc and pass here; the existing empty-axis tests are unchanged, since without all the behaviour is identical.

On your second question — whether rejecting ignored axes might be an intentional contract change — I do not think it can be, because all is already documented as bypassing all filtering and _validate itself warns rather than raises when all is combined with other tags. Rejecting one kind of ignored field while warning about another is not a coherent contract. If you did intend to tighten this, say so and I will withdraw the skip and update the documented all behaviour instead.

[Chen Yufeiyang (feiiiiii5)]

Both remaining points are addressed in c376fb0.

Hoisting has_all_tag over the singular-field check. Agreed, and it was inconsistent rather than intentional: after ba3ddbee the tag bypassed the empty-axis check but still raised here, while the warning twelve lines below says strict_match has no effect with all. Raising and then warning that the flag does nothing is self-contradictory, so the guard now reads if self.strict_match and not self.has_all_tag.

>>> SeedDatasetFilter(tags={"all"}, size={"small", "large"}, strict_match=True)
ValueError: strict_match=True with multiple values for 'size' ...
>>> # on c376fb0, and on base before this PR:
SeedDatasetFilter(tags={"all"}, size={"small", "large"}, strict_match=True)   # constructs

The wording. Correct, and thanks for catching that the split is the reason the guard exists at all. It now reads: Filter axes [...] were given an empty set. Without strict_match that matches no dataset, and with strict_match it matches every dataset that declares the axis. The same correction is in the docstring, which now says both checks are skipped under all.

New tests, both failing on ba3ddbee and passing here:

• test_all_tag_keeps_multi_valued_singular_fields — your exact input
• test_multi_valued_singular_fields_still_raise_without_all — the control, so skipping under all cannot become skipping generally
• test_empty_axis_message_covers_both_strict_match_outcomes — asserts both halves of the new wording, so it cannot drift back to the unconditional claim

pytest tests/unit/datasets/test_seed_dataset_metadata.py -q
  ba3ddbee     2 failed, 55 passed
  c376fb0      57 passed
ruff check / ruff format --check on both changed files    clean

The three existing empty-axis tests match on "empty set", which the new message still contains, so none needed changing.

[Chen Yufeiyang (feiiiiii5)]

Both should-fixes from Roman Lutz (@romanlutz) and the wording follow-up are now in c376fb0:

• The empty-axis check now returns [] when the 'all' tag is present, and the strict_match singular-field check is guarded by the same flag, so SeedDatasetFilter(tags={"all"}, size=..., strict_match=True) no longer raises – it again reaches the documented all-override path.
• The raise message now covers both branches of the split you asked for: "Without strict_match that matches no dataset, and with strict_match it matches every dataset that declares the axis." The same correction is reflected in the comment above it and the PR description.

hannahwestra25 re the earlier "all skips the empty-axis check... hoist has_all_tag over both": yes — now the empty-axes scan itself returns [] when has_all_tag is set, so both guards hoist over the override together.

[hannahwestra25]

thanks for contributing !