Skip to content

Fix Windows drive-letter paths misparsed as cloud schemes#986

Open
Abhishek21g wants to merge 1 commit into
mosaicml:mainfrom
Abhishek21g:fix/streaming-960-windows-paths
Open

Fix Windows drive-letter paths misparsed as cloud schemes#986
Abhishek21g wants to merge 1 commit into
mosaicml:mainfrom
Abhishek21g:fix/streaming-960-windows-paths

Conversation

@Abhishek21g

Copy link
Copy Markdown

Description of changes:

On Windows, absolute paths like D:/test are rejected by CloudUploader with ValueError: Invalid Cloud provider prefix: d. because urllib.parse.urlparse treats the drive letter as a URL scheme (scheme='d'). That breaks MDSWriter(out=r'D:/...') (issue #960).

Root cause: CloudUploader.get / _validate / __init__ used urlparse(...).scheme directly and looked it up in UPLOADERS.

This PR adds _provider_prefix(uri) which:

  • maps single alphabetic schemes (Windows drive letters) to ''LocalUploader
  • preserves existing dbfs:/Volumes handling
  • leaves real cloud schemes (s3, gs, …) unchanged

get, _validate, and __init__ all use the helper so drive-letter paths are not treated as remote.

Testing:

  • Extended test_instantiation_type with D:/... and C:/... local cases
  • New test_windows_drive_letter_not_cloud_scheme asserts _provider_prefix behavior + LocalUploader instantiation
  • pytest -vv tests/test_upload.py::TestCloudUploader --no-cov — 9 related tests passed locally

Issue #, if available:

Merge Checklist:

General

  • I have read the contributor guidelines
  • This is a documentation change or typo fix. If so, skip the rest of this checklist.
  • I certify that the changes I am introducing will be backward compatible, and I have discussed concerns about this, if any, with the MosaicML team.
  • I have updated any necessary documentation, including README and API docs (if appropriate).

Tests

  • I ran pre-commit on my change.
  • I have added tests that prove my fix is effective or that my feature works (if appropriate).
  • I ran the tests locally to make sure it pass.
  • I have added unit and/or integration tests as appropriate to ensure backward compatibility of the changes.

urlparse treats D:/path as scheme='d', so CloudUploader rejected absolute
Windows paths as invalid cloud providers. Map single-letter schemes to the
local uploader and use the same helper in get/validate/init (issue mosaicml#960).
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.

Windows absolute paths with drive letters incorrectly parsed as cloud provider schemes

1 participant