fix(dataset): raise a clear error on an empty dataset entry

[he-yufeng]

What

DatasetSyntax.parse('') crashes with a cryptic TypeError: stat: path should be string, bytes, os.PathLike or integer, not NoneType. An empty string falls through os.path.exists('') (False) into _safe_split('', '::', ...), which returns (None, None) for an empty string, so dataset becomes None and the next os.path.exists(dataset) gets None.

An empty entry is easy to hit: swift sft --dataset "$DS1" "$DS2" where a variable is unset but quoted parses $DS2 into a stray empty string in the --dataset list. BaseArguments.load_dataset only guards if self.dataset: (the list is non-empty), not each element, and load_dataset calls DatasetSyntax.parse per entry with no filter. The crash also happens before any --strict handling, so it hard-fails regardless.

Fix

Reject an empty entry at the top of parse with a clear, actionable ValueError. The entry is stripped first, so a whitespace-only string (which previously parsed through to an empty dataset id) raises the same error; note this also means a padded entry like " alpaca " is now trimmed rather than handled verbatim. Other entries are unaffected.

Test

test_dataset_syntax_rejects_empty_entry asserts DatasetSyntax.parse('') raises ValueError (previously a TypeError). I confirmed the crash and the fix by extracting the parse/_safe_split logic standalone (the local checkout cannot import swift without datasets/torch); the added test runs in CI.

[gemini-code-assist]

Code Review

This pull request adds validation to reject empty dataset entries in DatasetSyntax.parse with a clear ValueError and includes a corresponding unit test. The review feedback suggests enhancing this validation by stripping leading and trailing whitespace to also catch whitespace-only inputs, and expanding the unit test to cover these cases.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

Checking if not dataset: only handles empty strings or None, but does not handle strings that contain only whitespace (e.g., " "). A whitespace-only string will bypass this check and cause a failure later.

Additionally, stripping the dataset string at the very beginning of parse instead of only at the end (line 82) prevents potential bugs where leading or trailing whitespaces interfere with os.path.exists() checks or prefix matching (e.g., " ms::dataset" would fail to match "ms" because of the leading space).

Suggested change

	if not dataset:
	raise ValueError('Received an empty dataset entry. Check `--dataset`/`--val_dataset` for a stray empty '
	'string (e.g. an unset shell variable wrapped in quotes).')
	if isinstance(dataset, str):
	dataset = dataset.strip()
	if not dataset:
	raise ValueError('Received an empty dataset entry. Check `--dataset`/`--val_dataset` for a stray empty '
	'string (e.g. an unset shell variable wrapped in quotes).')

[he-yufeng]

Fixed in 65ffb8a — strip the entry at the top of parse so a whitespace-only string collapses to empty and hits the same ValueError, and updated the message to say "empty or whitespace-only".

[gemini-code-assist]

Expand the test to cover whitespace-only inputs (such as spaces, tabs, and newlines) to ensure they are also correctly rejected with a clear ValueError instead of causing issues down the line.

Suggested change

	def test_dataset_syntax_rejects_empty_entry():
	import pytest
	from swift.dataset.dataset_syntax import DatasetSyntax
	# An empty dataset entry (e.g. an unset shell variable wrapped in quotes in a
	# `--dataset "$DS1" "$DS2"` invocation) must raise a clear ValueError instead
	# of crashing deep inside with a cryptic `os.path.exists(None)` TypeError.
	with pytest.raises(ValueError, match='empty dataset entry'):
	DatasetSyntax.parse('')
	def test_dataset_syntax_rejects_empty_entry():
	import pytest
	from swift.dataset.dataset_syntax import DatasetSyntax
	# An empty dataset entry (e.g. an unset shell variable wrapped in quotes in a
	# `--dataset "$DS1" "$DS2"` invocation) must raise a clear ValueError instead
	# of crashing deep inside with a cryptic `os.path.exists(None)` TypeError.
	for empty_input in ('', ' ', ' \n\t '):
	with pytest.raises(ValueError, match='empty dataset entry'):
	DatasetSyntax.parse(empty_input)

[he-yufeng]

Done in 65ffb8a — the test now loops over ('', ' ', '\t', '\n') and asserts each raises the clear ValueError.

[ErenAta16]

Guarding this at the boundary is the right call, and the error text naming the shell-variable case is genuinely helpful, since --dataset "$DS1" "$DS2" with one variable unset is exactly how people hit it.

One correction to the description and the test comment, because it will send a maintainer looking for a crash that does not happen. Both say an empty entry crashes with a cryptic os.path.exists(None) TypeError. For an empty or whitespace-only string it does not:

os.path.exists('')      # False
os.path.exists('   ')   # False
os.path.exists(None)    # TypeError: path should be string, bytes, os.PathLike or integer, not NoneType

parse opens with if os.path.exists(dataset), so '' simply takes the else branch and flows on through _safe_split as an empty name. The TypeError only appears if dataset is literally None, which is a different input from the one this PR guards and one the new check would not catch either, since if dataset: is False for None and the raise happens before anything dereferences it. That part works out, it is just not what the comment says.

So the real failure on main is quieter than described: the empty entry is accepted, carried downstream, and fails later at whatever first tries to resolve it, with a message that does not mention which --dataset argument was empty. That is a better argument for this fix than a TypeError would be, because a loud crash at least points somewhere, and I would put it in the description in place of the current claim.

Two small things on the check itself:

if dataset:
    dataset = dataset.strip()
if not dataset:
    raise ValueError(...)

The guard is only reached for a truthy value, so the first if is doing nothing the second cannot: dataset = dataset.strip() if dataset else dataset collapses to the same two outcomes. if not dataset or not dataset.strip(): says it in one condition, or keep the strip and drop the outer guard, since ''.strip() is safe.

The stripped value is assigned to the local but the function continues with it, so a padded entry like " alpaca " is now silently trimmed rather than rejected. That is almost certainly what you want, but it is a second behaviour change riding along with the error, and it is worth one line in the description.

Test covers '', spaces, tab and newline, which is the right set.

[he-yufeng]

Thanks for the careful read. On the crash question I double checked on current main, and it does crash, just one step later than your trace: os.path.exists('') is False, then _safe_split('', '::', ...) returns (None, None) for an empty string, so dataset becomes None and the next os.path.exists(dataset) raises TypeError: stat: path should be string, bytes, os.PathLike or integer, not NoneType. Ran it against a clean checkout of main to be sure. A whitespace-only entry is the different story you describe: no crash, it parses through to an empty dataset id, which is why the strip matters beyond the crash fix.

Both style points taken in b417248: the guard is now a plain dataset.strip() followed by the emptiness check (''.strip() is safe), and the description calls out that padded entries like " alpaca " are trimmed rather than rejected.

[ErenAta16]

You are right and my trace named the wrong line. Replayed parse('') against current main step by step:

line 59  os.path.exists('')                -> False
line 62  _safe_split('', '::', False)      -> (None, None)   so dataset = None
line 66  os.path.exists(None)              -> TypeError: _path_exists: path should be string,
                                              bytes, os.PathLike or integer, not NoneType

_safe_split short-circuits on the empty string before it ever splits:

if s is None or len(s) == 0:
    return None, None          # dataset_syntax.py:40-41

so the empty string is laundered into None and the crash lands one os.path.exists later than I said.

Worth recording why it crashes rather than quietly returning False, since that is the part that makes the failure ugly instead of merely wrong: os.path.exists swallows OSError and ValueError and returns False, but not TypeError. A malformed path string is absorbed; a None propagates. So the user sees a TypeError from inside posixpath with no mention of the dataset argument they actually passed, which is exactly the diagnostic problem this PR is fixing.

Stripping at the top of parse so whitespace-only collapses to empty and hits the same ValueError is the right shape, and looping the test over ('', ' ', '\t', '\n') covers the spellings that would otherwise each need their own discovery. Thanks for checking the trace rather than taking mine at face value.

[he-yufeng]

Appreciate you re-running it step by step. The os.path.exists asymmetry (absorbs OSError/ValueError, propagates TypeError) is exactly why the empty entry surfaces as a cryptic posixpath error instead of a dataset message, which is what the guard is for. Leaving it here then; happy to adjust anything else if it comes up in review.