Skip to content

MAINT: Deprecate dead split kwarg on 8 single-split HF dataset loaders#1901

Merged
romanlutz merged 3 commits into
microsoft:mainfrom
romanlutz:romanlutz/audit-hf-dataset-split-args
Jun 3, 2026
Merged

MAINT: Deprecate dead split kwarg on 8 single-split HF dataset loaders#1901
romanlutz merged 3 commits into
microsoft:mainfrom
romanlutz:romanlutz/audit-hf-dataset-split-args

Conversation

@romanlutz
Copy link
Copy Markdown
Contributor

Description

Eight _*Dataset loaders under pyrit/datasets/seed_datasets/remote/ expose a split: str constructor kwarg that forwards to _fetch_from_huggingface, even though the upstream HuggingFace dataset publishes only one split. The kwarg has exactly one valid value, never does anything useful, and is a misleading API surface that suggests users can pick a split when they can't.

Because the kwarg is part of the public constructor signature, this PR deprecates rather than deletes it (target removal: v0.16.0, matching the existing cohort in seed_dataset_provider.py and prepended_conversation_config.py):

  • split: str = "<value>"split: str | None = None
  • DeprecationWarning(stacklevel=2) when a non-None value is passed
  • self.split dropped; the single literal split name is hardcoded at the _fetch_from_huggingface(..., split="<value>") call site
  • constructor docstring marks split as deprecated

Loaders affected (with the hardcoded literal):

Class HF dataset Literal
_CBTBenchDataset Psychotherapy-LLM/CBT-Bench (38 configs, each single-split) "train"
_DarkBenchDataset apart/darkbench "train"
_ForbiddenQuestionsDataset TrustAIRLab/forbidden_question_set config="default", split="train"
_HarmfulQADataset declare-lab/HarmfulQA "train"
_HiXSTestDataset walledai/HiXSTest (gated) "train"
_ORBench{80K,Hard,Toxic}Dataset bench-llm/OR-Bench (one base-class edit) "train"
_SGXSTestDataset walledai/SGXSTest (gated) "train"
_SimpleSafetyTestsDataset Bertievidgen/SimpleSafetyTests "test" (note: not "train")

_ForbiddenQuestionsDataset is a special case: the kwarg was actually misforwarded to HuggingFace as config= rather than split=, but TrustAIRLab/forbidden_question_set only has one config ("default") and one split ("train"), so the bug was harmless. The deprecation message explains both issues.

Out of scope: multi-split loaders where the split kwarg has more than one valid value — _BeaverTailsDataset, _SaladBenchDataset, _ToxicChatDataset, _JBBBehaviorsDataset. Whether those should be refactored into sibling subclasses per the "each distinct upstream artifact is its own registered subclass" rule is a separate design discussion.

Tests and Documentation

  • Each affected test file gets a test_split_kwarg_emits_deprecation_warning case asserting the warning fires; existing tests that previously forwarded a non-default split= are updated to drop the kwarg (the hardcoded literal is still asserted on the _fetch_from_huggingface call kwargs).
  • Unit tests: all 64 tests in the 8 affected test files pass, including the 9 new deprecation tests.
  • End-to-end tests: tests/end_to_end/test_all_datasets.py was run scoped to the 10 affected providers (8 loader entries + 2 extra OR-Bench siblings) and all 10 successfully fetched real data from HuggingFace, including the 2 gated datasets (HiXSTest, SGXSTest) and the large OR-Bench 80K.
  • Pre-commit (ruff format, ruff check, ty type check) passes on every modified file.
  • Grep across pyrit/, tests/, and doc/ confirmed no other call sites pass split= to these loaders — only the new deprecation tests do.
  • No documentation changes needed: the affected loaders are private classes (leading underscore), their constructor kwarg is not referenced from the rendered notebooks or other docs, and the deprecation message is self-documenting.

romanlutz and others added 3 commits June 2, 2026 15:41
These loaders expose a `split: str` constructor kwarg that forwards to
_fetch_from_huggingface, but the upstream HuggingFace dataset publishes
only one split per (loader, config). The kwarg has exactly one valid
value and is a misleading API surface that suggests users can pick a
split when they can't.

Per the public-API constraint (the kwarg is constructor-visible), this
change deprecates rather than deletes:

  - constructor signature changes from `split: str = "<value>"` to
    `split: str | None = None`
  - DeprecationWarning (stacklevel=2) is emitted when a non-None value
    is passed; target removal is v0.16.0, matching the existing cohort
    in seed_dataset_provider.py and prepended_conversation_config.py
  - self.split is dropped; the (single) literal split name is hardcoded
    at the _fetch_from_huggingface(..., split="<value>") call site
  - constructor docstrings now mark `split` as deprecated

Loaders affected (with the hardcoded literal):

  - _CBTBenchDataset  -> split="train" (38 configs, each single-split)
  - _DarkBenchDataset -> split="train"
  - _ForbiddenQuestionsDataset -> config="default", split="train" (also
    fixes a misforwarding: the kwarg was passed as config= rather than
    split=, but TrustAIRLab/forbidden_question_set only has one of each,
    so it never did anything useful)
  - _HarmfulQADataset -> split="train"
  - _HiXSTestDataset  -> split="train" (gated)
  - _ORBench{80K,Hard,Toxic}Dataset -> split="train" (one edit on the
    shared _ORBenchBaseDataset)
  - _SGXSTestDataset  -> split="train" (gated)
  - _SimpleSafetyTestsDataset -> split="test" (this dataset is the odd
    one out: its single published split is "test", not "train")

Multi-split loaders (BeaverTails, SaladBench, ToxicChat, JBBBehaviors)
are out of scope - their split kwarg has multiple valid values and is
doing real work. Whether they should be refactored into sibling
subclasses per the "each distinct upstream artifact is its own
registered subclass" rule is a separate design discussion.

Each affected test file gets a test_split_kwarg_emits_deprecation_warning
case asserting the warning fires; existing tests that previously
forwarded a non-default split= are updated to drop the kwarg (the
hardcoded literal is still asserted on the _fetch_from_huggingface call
kwargs).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…taset-split-args

# Conflicts:
#	pyrit/datasets/seed_datasets/remote/or_bench_dataset.py
#	tests/unit/datasets/test_hixstest_dataset.py
#	tests/unit/datasets/test_sgxstest_dataset.py
The merge from origin/main brought in TestDarkBenchDataset.
test_fetch_dataset_with_custom_config (added in microsoft#1780), which
constructed _DarkBenchDataset(split="test") and asserted the kwarg
was forwarded to _fetch_from_huggingface_async. That contract no
longer holds: this PR deprecates the dead split kwarg and hardcodes
split="train" at the DarkBench call site, since upstream
apart/darkbench publishes only the "train" split.

Drop the deprecated kwarg from the constructor call and assert the
hardcoded "train" literal that actually flows through.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@romanlutz romanlutz added this pull request to the merge queue Jun 3, 2026
Merged via the queue into microsoft:main with commit 1a15cb8 Jun 3, 2026
52 checks passed
@romanlutz romanlutz deleted the romanlutz/audit-hf-dataset-split-args branch June 3, 2026 19:49
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.

2 participants