Skip to content

Fix dataset filters matching datasets that never declare the filtered axis - #2587

Open
WatchTree-19 wants to merge 1 commit into
microsoft:mainfrom
WatchTree-19:fix-undeclared-axis-matches-filter
Open

Fix dataset filters matching datasets that never declare the filtered axis#2587
WatchTree-19 wants to merge 1 commit into
microsoft:mainfrom
WatchTree-19:fix-undeclared-axis-matches-filter

Conversation

@WatchTree-19

Copy link
Copy Markdown
Contributor

The bug

SeedDatasetProvider._match_single_criterion skips a field when the dataset's metadata for it is None:

for field in dc_fields(SeedDatasetMetadata):
    filter_vals = getattr(criterion, field.name)
    meta_vals = getattr(metadata, field.name)

    if filter_vals is None or meta_vals is None:
        continue

meta_vals is None means the dataset never declared that axis, which is not the same as declaring it and matching. Skipping drops the filtered axis entirely, so the method returns True for every dataset that is simply silent about it.

Why this is a bug rather than a design choice

Two things in the file say so.

The docstring on this method (Match a dataset's metadata against filter criteria):

Within each criterion, ALL specified fields must match (AND across fields).

And get_all_dataset_names_async, seven lines earlier, makes the opposite call for the neighbouring case:

# Datasets without metadata are skipped for all other filters
if not metadata:
    continue

A dataset carrying no metadata is excluded. A dataset carrying partial metadata that is silent on the filtered axis is included. A dataset that says nothing at all is treated as less qualified than one that says nothing about the thing you asked for.

The skip also happens before the strict_match split, so strict_match=True is affected identically — the stricter mode is no stricter here.

Reproduction

Running the matcher verbatim against a dataset that declares tags, size and source_type but is silent on modalities and harm_categories:

case                                                        current   correct
filter modalities={audio} vs dataset SILENT on modalities      True     False
filter modalities={audio} vs dataset DECLARING {text,image}   False     False
filter harm_categories={nonexistent} vs SILENT                 True     False
same, strict_match=True                                        True     False
filter tags={safety} vs dataset DECLARING tags                 True      True

The second and fifth rows are controls: the check behaves correctly the moment the dataset declares the axis. This is purely the None path.

Scale

839 of the 840 YAML dataset files under pyrit/datasets never declare modalities. Every one of them currently satisfies a modalities filter. The same shape applies to harm_categories, which most locally-registered datasets also leave unset.

get_all_dataset_names_async is the public discovery entry point and feeds fetch_datasets_async, so in practice this is a user filtering for audio datasets, or for one harm category, and being handed a set dominated by datasets that say nothing about either. Nothing is logged.

The change

Separate the two None cases. A filter that does not name an axis still skips it; a dataset that does not declare a named axis no longer matches it.

Tests

Three added to TestMetadataParsingRemote, all failing on main:

  • an undeclared axis does not match, for both modalities and harm_categories, while a declared axis still matches normally
  • the same under strict_match=True
  • an undeclared axis does not mask a declared mismatch when two axes are filtered at once

Nothing in the existing suite locked in the old behaviour — the metadata-filter and strict-match tests all construct metadata that declares the axis under test.

What I did not verify

I did not run the full unit suite, only tests/unit/datasets. I also have not confirmed whether "undeclared axis is a wildcard" was ever the intent; I argue against it from the docstring and from the if not metadata: continue precedent, but if it was deliberate then the docstring and that neighbouring branch are the things that need changing instead, and I would rather be told.

Written with AI assistance; I have read and can explain every line.

… axis

_match_single_criterion skips a field when the dataset's metadata for it is
None:

    if filter_vals is None or meta_vals is None:
        continue

meta_vals is None means the dataset never declared that axis, which is not the
same as declaring it and matching. Skipping drops the filtered axis entirely, so
the function returns True for every dataset that is simply silent about it.

That contradicts the method's own docstring - "Within each criterion, ALL
specified fields must match (AND across fields)" - and it contradicts
get_all_dataset_names_async seven lines earlier, which excludes datasets that
carry no metadata at all:

    # Datasets without metadata are skipped for all other filters
    if not metadata:
        continue

So a dataset with partial metadata was treated as better qualified than one
with none.

It fires on both branches, because the skip happens before the strict_match
split, so strict_match=True is affected identically.

Scale: 839 of the 840 YAML dataset files under pyrit/datasets never declare
`modalities`, and every one of them currently satisfies a `modalities` filter.
Filtering on a harm category or a modality returns a set dominated by datasets
that say nothing about either, and nothing is logged.

get_all_dataset_names_async is the public discovery entry point and feeds
fetch_datasets_async, so this is a user asking for audio datasets and being
handed text ones.

Adds three tests: an undeclared axis does not match, the same under
strict_match, and an undeclared axis does not mask a declared mismatch when two
axes are filtered at once.
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.

1 participant