Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Spot/Storage] Supports empty spaces in source path for storages #2835

Merged

Conversation

landscapepainter
Copy link
Collaborator

@landscapepainter landscapepainter commented Dec 3, 2023

This PR resolves #2763 and #2243.

When a source path specified to file_mounts in task YAML had an empty space, Skypilot failed to upload the source to the cloud storages due to the limitation of CLIs used(#2243). This issue also prevented users from specifying workdir with empty spaces when running managed spot jobs as well since workdir is uploaded to the cloud storage first(#2763).

This PR supports the use of empty spaces in the file/directory names for source in file_mounts and workdir for managed spot jobs. And a test test_upload_source_with_spaces is added to check such cases.

Thanks to @aseriesof-tubes for the contribution.

Tested (run the relevant ones):

file_mounts:
  /doyoung-testing:
    name: doyoung-testing
    source: [~/yaml/doyoung-test.yaml, '/home/doyoung/path with spaces/file with spaces']
    store: gcs

  • Manual test with task YAML 2:
file_mounts:
  /gcs-mount: 
    name: source-with-space
    source: /home/doyoung/source with space
    store: gcs

Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the issue @landscapepainter! LGTM. It should be good to go in once the tests for storage have passed.

@landscapepainter landscapepainter merged commit 76c5af9 into skypilot-org:master Dec 8, 2023
19 checks passed
remyleone pushed a commit to remyleone/skypilot that referenced this pull request Dec 26, 2023
…pilot-org#2835)

* for issue 2243

* comments and formatting

* resolve for path with empty spaces

* revert changes as adopting shlex.quote

* update smoke test

* format

* nit

---------

Co-authored-by: Andrew Kim <andrewkim@Andrews-MacBook-Pro-10.local>
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.

[Spot] Spot launch with a workdir path that contains spaces fails
2 participants