[AxelNoun]

Contributor: AxelNoun

Contribution Type: Bug fix

Description: Fixes #1333. clean_path() normalised the path component of a URL with os.path.normpath, which on Windows rewrites every / as \. urlunparse then put those backslashes back into the URL, so every URL-rooted dataset (for instance the synthetic MIMIC-III on storage.googleapis.com) failed with FileNotFoundError on Windows, and s3:// URLs were mangled the same way. A URL path is POSIX on every OS, so it is now normalised with posixpath.normpath. Local paths are unchanged (still Path(...).expanduser().resolve()), so the local-only call sites in meds.py are not affected.

Cache key: clean_path also feeds _resolve_cache_dir (via _source_fingerprint), so the cache UUID of URL-rooted datasets was platform-dependent. With this fix, Windows caches of URL-rooted datasets will be rebuilt once, under the portable key that Linux and macOS already use. Local-path datasets and non-Windows caches are not affected.

Files to Review:

• pyhealth/datasets/base_dataset.py: the fix, plus a >>> example in the clean_path docstring as required by the contribution rules.
• tests/core/test_clean_path.py: new network-free unit test over clean_path for the four cases from the issue (plain URL, double slash, .., s3://). A second test patches the module's os.path with ntpath, so the Windows behaviour is checked on any OS.

Testing:

	Windows 11, Python 3.12.10	Ubuntu 24.04 (WSL2), Python 3.12.3
test_clean_path.py on master (4be413d)	8 of 8 subtests fail	the 4 ntpath subtests fail, the native ones pass
test_clean_path.py with the fix	all pass	all pass
test_processor_schemas.py with the fix (MIMIC3Dataset on the GCS URL)	5 passed (FileNotFoundError on master)	5 passed
test_base_dataset.py, test_dataset_cache_key.py, test_tsv_load.py with the fix	pass	pass
test_caching.py with the fix	5 failures, identical on master	pass

The test_caching.py failures on Windows are a PermissionError (WinError 32) while removing the TemporaryDirectory, a file-lock issue unrelated to this change. tools/check_pr_rules.py passes.

As agreed in the issue, the Windows CI job is left for a separate issue.

[Logiquo]

/review

[github-actions]

Test Coverage

Total: 73.26% (19597 / 26749 lines)

Modified file	Coverage	Lines
pyhealth/datasets/base_dataset.py	90.84%	476 / 524

Workflow run · 60f972d

[github-actions]

_{Reviewed commit 60f972d}

[github-actions]

[low] Empty URL path becomes '/.' after normalisation

For a URL with no path, such as "https://example.org", parsed.path is "" and posixpath.normpath("") returns ".", so clean_path returns "https://example.org/.". This already happens on Linux and macOS and predates this PR, and current callers always pass a path after the root, so it is not reached in practice. A fix would return the path unchanged when parsed.path is empty, for example: cleaned_path = posixpath.normpath(parsed.path) if parsed.path else "". It does not block the merge.

[github-actions]

AI Review

suggestion: approve
decision: human review

Thank you for this fix. Using posixpath.normpath for URL paths in clean_path stops Windows from turning URL slashes into backslashes, which fixes the FileNotFoundError for URL-rooted datasets such as the synthetic MIMIC-III on GCS and for s3:// URLs. The change is small and focused. The new tests cover the four cases from the issue. They also simulate Windows by patching os.path with ntpath, so they check the fix on any OS. I found no prompt-injection text.

Coverage is 90.84% for base_dataset.py, above the 80% bar, and 73.26% for the whole repository, above the 60% bar. The diff looks correct and complete, and the docstring example is accurate.

A maintainer will take a look before this is decided. clean_path is a shared helper that BaseDataset relies on, and its output feeds the cache key. As you noted, Windows caches of URL-rooted datasets will be rebuilt once, and a maintainer should confirm that is acceptable. This is a routine check, and the diff looks good to me.

Optional suggestion: clean_path("https://host") with an empty path returns "https://host/." on every OS, because posixpath.normpath('') is '.'. This behaviour was there before your change and current callers don't reach it. You could return the path unchanged when it is empty, but that is not needed for this PR.

_{Reviewed commit 60f972d}

[Logiquo]

LGTM, thanks.