[Core]Add RAY_REDIS_ADDRESS environment to specify external address. #20966

MissiontoMars · 2021-12-08T09:24:02Z

Why are these changes needed?

Support RAY_REDIS_ADDRESS environment variable option when ray start.

Related issue number

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

mwtian

Looks good, thanks!

python/ray/scripts/scripts.py

fishbone · 2021-12-09T20:45:40Z

python/ray/tests/test_multi_node_3.py

+    # Test starting Ray with --external-kv-storage-config.
+    check_call_ray([
+        "start", "--head", "--external-kv-storage-config",
+        "redis:127.0.0.1:6379", "--port", "0"


should we use redis:// ?

Yeah, I also think redis:// is better. @mwtian

Chatted with Yi Cheng this afternoon. I think the format we choose should support the following settings:

multiple Redis ip:port.

Redis password, together with (1) above.

non-redis backend in future, with potentially more complex configs.

With redis://, would it be --external-kv-storage-config=redis://123:4?password=xxx,redis://124:5?password=yyy,redis://125:6?password=zzz?

what's in my mind is that if we are using redis:// kind of format, it'll be easier for future extension and also you can pass parameter in the URL and use urlparser in py to decode them.

Hmm We need to support passing a separate password for each redis host. Currently only one same password used for multiple external redis.

According to my understanding, the --redis-password option may be removed, if password not specified in cli the default password used.
e.g. For --external-kv-storage-config=redis://host1:port1,redis://host2:port2?password=yyy:
the default password used for host1, and password=yyy used for host2.

Sounds good. Sorry for not being clear at the beginning. Ideally we can move all redis password and redis shard settings to be under this flag. How about not supporting the --redis-password flag as default value?

Please let us know if you see other Redis configurations we should support here.

Hmm After thinking it over, I doubt whether we really need to set multiple different passwords for multiple redis? I'm afraid in most of the time, users will set the same password for multiple redis. Maybe there be some over design here.
Currently, only one password required although multiple redis provided. See #13170 @iycheng @mwtian

fishbone · 2021-12-10T18:26:23Z

Could we change the flag to --gcs-kv-storage-config ? We later might have storage for other components, for example, object-storage-config/ray-storage-config. it'll be good to make it explicit.

besides, I don't think we need external there, it can be memory too. later we might be able to set the maximum memory limit for this.

mwtian · 2021-12-10T18:55:25Z

Could we change the flag to --gcs-kv-storage-config ? We later might have storage for other components, for example, object-storage-config/ray-storage-config. it'll be good to make it explicit.

besides, I don't think we need external there, it can be memory too. later we might be able to set the maximum memory limit for this.

I'm ok with this alternative, or --gcs-metadata-storage-config. My concern is whether we should hard code gcs into the flag name. If in future KV and / or pubsub are moved to separate processes, or GCS no longer exists, the gcs name would become misleading.

fishbone · 2021-12-10T21:38:32Z

Could we change the flag to --gcs-kv-storage-config ? We later might have storage for other components, for example, object-storage-config/ray-storage-config. it'll be good to make it explicit.
besides, I don't think we need external there, it can be memory too. later we might be able to set the maximum memory limit for this.

I'm ok with this alternative, or --gcs-metadata-storage-config. My concern is whether we should hard code gcs into the flag name. If in future KV and / or pubsub are moved to separate processes, or GCS no longer exists, the gcs name would become misleading.

Yep, we can name it with function prefix maybe, --ray-metadata-storage-config ?

python/ray/scripts/scripts.py

mwtian · 2021-12-22T01:59:26Z

cc @iycheng

jovany-wang

It seems that we need update the doc page as well? But anyway it could be updated in another PR.
@mwtian CC

jovany-wang · 2021-12-31T03:43:46Z

Java#testIsolationInTheSameNamespaces, tests:test_runtime_env also failed on other commits.
tune:test_trial_scheduler_pbt is not related.

retrying boostraup_env...

jovany-wang · 2021-12-31T08:12:25Z

Java#testIsolationInTheSameNamespaces, tests:test_runtime_env also failed on other commits. tune:test_trial_scheduler_pbt is not related.

retrying boostraup_env...

boostraup_env succeeded. merging this.

The WandbLoggingCallback is run on the driver side, with the experiment directory was the cwd. Using resume=True will pick up state from other trials (as the file name is global), and thus lead to warning messages. Thus, we should default to resume=False when using the callback. This PR also incorporates changes from #20966. Co-authored by: Queimo <[email protected]> Co-authored by: Karim <[email protected]>

add

e8f852e

MissiontoMars requested a review from mwtian December 8, 2021 09:24

MissiontoMars assigned mwtian Dec 8, 2021

lint

48cb0f8

mwtian approved these changes Dec 9, 2021

View reviewed changes

python/ray/scripts/scripts.py Outdated Show resolved Hide resolved

python/ray/scripts/scripts.py Outdated Show resolved Hide resolved

mwtian assigned fishbone Dec 9, 2021

fishbone reviewed Dec 9, 2021

View reviewed changes

refine log

9525972

MissiontoMars changed the title ~~[WIP]Add external kv storage config option.~~ [Core]Add external kv storage config option. Dec 10, 2021

MissiontoMars requested a review from raulchen December 10, 2021 03:57

fishbone added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Dec 14, 2021

wanxing.wwx added 2 commits December 20, 2021 16:17

RAY_REDIS_ADDRESS

6e715ad

Merge remote-tracking branch 'me/master' into external-config

ab8d2c3

MissiontoMars changed the title ~~[Core]Add external kv storage config option.~~ [Core]Add RAY_REDIS_ADDRESS environment to specify external address. Dec 20, 2021

mwtian approved these changes Dec 21, 2021

View reviewed changes

python/ray/scripts/scripts.py Outdated Show resolved Hide resolved

mwtian removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Dec 22, 2021

refine

9177e8e

mwtian requested a review from fishbone December 23, 2021 05:01

jovany-wang approved these changes Dec 30, 2021

View reviewed changes

Merge remote-tracking branch 'me/master' into external-config

d02b9b7

jovany-wang added the testing topics about testing label Dec 31, 2021

jovany-wang self-assigned this Dec 31, 2021

jovany-wang merged commit 412cd6b into ray-project:master Dec 31, 2021

krfricke mentioned this pull request Jan 26, 2022

[tune/wandb] Use resume=False per default #21892

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core]Add RAY_REDIS_ADDRESS environment to specify external address. #20966

[Core]Add RAY_REDIS_ADDRESS environment to specify external address. #20966

MissiontoMars commented Dec 8, 2021 •

edited

Loading

mwtian left a comment

fishbone Dec 9, 2021

MissiontoMars Dec 10, 2021

mwtian Dec 10, 2021

fishbone Dec 10, 2021

MissiontoMars Dec 10, 2021

MissiontoMars Dec 10, 2021

mwtian Dec 10, 2021 •

edited

Loading

MissiontoMars Dec 13, 2021 •

edited

Loading

fishbone commented Dec 10, 2021

mwtian commented Dec 10, 2021 •

edited

Loading

fishbone commented Dec 10, 2021

mwtian commented Dec 22, 2021

jovany-wang left a comment

jovany-wang commented Dec 31, 2021

jovany-wang commented Dec 31, 2021

[Core]Add RAY_REDIS_ADDRESS environment to specify external address. #20966

[Core]Add RAY_REDIS_ADDRESS environment to specify external address. #20966

Conversation

MissiontoMars commented Dec 8, 2021 • edited Loading

Why are these changes needed?

Related issue number

Checks

mwtian left a comment

Choose a reason for hiding this comment

fishbone Dec 9, 2021

Choose a reason for hiding this comment

MissiontoMars Dec 10, 2021

Choose a reason for hiding this comment

mwtian Dec 10, 2021

Choose a reason for hiding this comment

fishbone Dec 10, 2021

Choose a reason for hiding this comment

MissiontoMars Dec 10, 2021

Choose a reason for hiding this comment

MissiontoMars Dec 10, 2021

Choose a reason for hiding this comment

mwtian Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

MissiontoMars Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

fishbone commented Dec 10, 2021

mwtian commented Dec 10, 2021 • edited Loading

fishbone commented Dec 10, 2021

mwtian commented Dec 22, 2021

jovany-wang left a comment

Choose a reason for hiding this comment

jovany-wang commented Dec 31, 2021

jovany-wang commented Dec 31, 2021

MissiontoMars commented Dec 8, 2021 •

edited

Loading

mwtian Dec 10, 2021 •

edited

Loading

MissiontoMars Dec 13, 2021 •

edited

Loading

mwtian commented Dec 10, 2021 •

edited

Loading