Skip to content

[Fix] alpaca_enzh_dataset passes a shuffle_before_pack kwarg neither callee accepts - #2057

Open
Anai-Guo wants to merge 1 commit into
InternLM:mainfrom
Anai-Guo:fix-alpaca-enzh-shuffle-kwarg
Open

[Fix] alpaca_enzh_dataset passes a shuffle_before_pack kwarg neither callee accepts#2057
Anai-Guo wants to merge 1 commit into
InternLM:mainfrom
Anai-Guo:fix-alpaca-enzh-shuffle-kwarg

Conversation

@Anai-Guo

Copy link
Copy Markdown

Motivation

alpaca_enzh_dataset (xtuner/apis/datasets/alpaca.py) cannot be called at all — it raises TypeError on its very first statement.

It forwards shuffle_before_pack=True to alpaca_dataset and alpaca_zh_dataset:

alpaca = alpaca_dataset(
    tokenizer,
    path=path_en,
    max_length=max_length,
    prompt_template=prompt_template,
    shuffle_before_pack=True,          # <-- not a parameter of alpaca_dataset
    remove_unused_columns=remove_unused_columns,
    pack_to_max_length=pack_to_max_length,
)

but neither callee declares that parameter:

def alpaca_dataset(tokenizer, path="tatsu-lab/alpaca", max_length=2048,
                   prompt_template=PROMPT_TEMPLATE.default,
                   remove_unused_columns=True, pack_to_max_length=True):

so both calls fail with

TypeError: alpaca_dataset() got an unexpected keyword argument 'shuffle_before_pack'

alpaca_enzh_dataset and alpaca_enzh_data_collator are both re-exported from xtuner.apis.datasets, so this is reachable straight from the public API.

Modification

Drop the two shuffle_before_pack=True lines from alpaca_enzh_dataset.

This does not change behaviour. Both callees already hardcode shuffle_before_pack=True on their internal process_hf_dataset call (lines 69 and 97), so the packing shuffle stays on exactly as before — the argument was only ever redundant, and it was never accepted.

lawyer_dataset in the same package is the same aggregator pattern (two sub-datasets combined with ConcatDataset, forwarding tokenizer / max_length / prompt_template / remove_unused_columns / pack_to_max_length) and already omits shuffle_before_pack. This change makes alpaca_enzh_dataset match that sibling.

Verification

I do not have the training dependencies installed locally, so I verified this at the signature level: the real defs and the real call sites are extracted from the file with ast, an inspect.Signature is rebuilt from the parameter list, and each call site is replayed through Signature.bind().

Before:

def alpaca_dataset(tokenizer, path=None, max_length=None, prompt_template=None, remove_unused_columns=None, pack_to_max_length=None)
def alpaca_zh_dataset(tokenizer, path=None, max_length=None, prompt_template=None, remove_unused_columns=None, pack_to_max_length=None)

  L26 alpaca_dataset(...)    -> TypeError: got an unexpected keyword argument 'shuffle_before_pack'
  L35 alpaca_zh_dataset(...) -> TypeError: got an unexpected keyword argument 'shuffle_before_pack'

After:

  L26 alpaca_dataset(...)    -> OK
  L34 alpaca_zh_dataset(...) -> OK

alpaca_dataset:    process_hf_dataset(shuffle_before_pack=True)
alpaca_zh_dataset: process_hf_dataset(shuffle_before_pack=True)

The last two lines are the behaviour-equivalence check: the shuffle is still enabled inside both callees after the patch.

ruff check and ruff format --diff are clean against the repo's pyproject.toml config.

BC-breaking

No. The removed argument could never be passed successfully in the first place, and the effective shuffle_before_pack=True behaviour is unchanged.

Checklist

  • Pre-commit / code style — ruff check and ruff format --diff clean with the repo config (note: .pre-commit-config.yaml scopes its hooks to xtuner/v1/ and autotest/, which does not cover this path).
  • The modification is covered above, including why it is behaviour-preserving.
  • Does not modify any other module's behaviour.

🤖 Generated with Claude Code

…not pass

`alpaca_enzh_dataset` forwards `shuffle_before_pack=True` to both
`alpaca_dataset` and `alpaca_zh_dataset`, but neither accepts that
parameter, so every call raises

    TypeError: alpaca_dataset() got an unexpected keyword argument
    'shuffle_before_pack'

Both callees already hardcode `shuffle_before_pack=True` on their internal
`process_hf_dataset` call, so dropping the argument keeps the behaviour
identical. `lawyer_dataset` -- the other aggregator built the same way --
already omits it.
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