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

Double quote --storage argument before passing it to raylet #41678

Closed
wants to merge 3 commits into from

Conversation

tvildo
Copy link

@tvildo tvildo commented Dec 6, 2023

fixes #41568

Why are these changes needed?

when starting ray using --storage argument like that:

ray start --head  --dashboard-host=0.0.0.0 --block --storage="s3://explore/ray?&endpoint_override=http://192.168.0.3:9000"

Raylet on worker will crash

Related issue number

Closes #41568

@jjyao
Copy link
Collaborator

jjyao commented Dec 7, 2023

I think many tests are failing: https://buildkite.com/ray-project/premerge/builds/13876

@tvildo tvildo force-pushed the master branch 2 times, most recently from 22b95c4 to cb72401 Compare December 16, 2023 06:14
@tvildo
Copy link
Author

tvildo commented Dec 16, 2023

@jjyao All tests are passing now.

@@ -1676,7 +1676,7 @@ def start_raylet(
)

if storage is not None:
start_worker_command.append(f"--storage={storage}")
start_worker_command.append(f'--storage="{storage}"')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if you do f"--storage='{stroage}'", you may not need the strip change.

fixes ray-project#41568

Signed-off-by: Tvildo <davidtamuna@gmail.com>
Signed-off-by: tvildo <davidtamuna@gmail.com>
Escape storage arg before passing it to worker
Remove double quotes from parsed cmd storage arg before passing it downstream.

Signed-off-by: tvildo <davidtamuna@gmail.com>
@tvildo
Copy link
Author

tvildo commented Dec 17, 2023

@jjyao I tried with single quote and it still needs strip https://buildkite.com/ray-project/premerge/builds/15005 I am reverting back my changes

@tvildo tvildo force-pushed the master branch 2 times, most recently from 03d719a to 1ca9d50 Compare December 17, 2023 16:45
@jjyao
Copy link
Collaborator

jjyao commented Dec 17, 2023

I see. So it's a windows only issue? I saw tests passed for linux.

@tvildo
Copy link
Author

tvildo commented Dec 17, 2023

Yes. With strip it works in all environments.

@tvildo
Copy link
Author

tvildo commented Dec 20, 2023

@jjyao Can I do anything else to be able to merge this pull request ?

@girogisturua
Copy link

I have same issue, great solution, please, merge asap

@jjyao
Copy link
Collaborator

jjyao commented Jan 11, 2024

@tvildo I did more investigation and found the root cause and have a fix here: #42332. Could you try it out and see if it fixes your issue?

Copy link

stale bot commented Mar 17, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Mar 17, 2024
@anyscalesam
Copy link
Contributor

closing as old here per @jjyao above should fix this already - @tvildo can you confirm/feel to reopen if disagree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale The issue is stale. It will be closed within 7 days unless there are further conversation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

default_worker.py crashes when initializing --storage in ray head node
4 participants