[core] Store Internal Config in GCS by ijrsvt · Pull Request #8921 · ray-project/ray

ijrsvt · 2020-06-12T22:53:01Z

Why are these changes needed?

This stores a single Internal Config for the entire cluster in GCS. This removes the need for specifying the config on every subsequent node that is added.

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/latest/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failure rates at https://ray-travis-tracker.herokuapp.com/.
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested (please justify below)

AmplabJenkins · 2020-06-12T22:54:24Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27096/
Test FAILed.

AmplabJenkins · 2020-06-12T22:56:57Z

Can one of the admins verify this patch?

AmplabJenkins · 2020-06-12T23:09:00Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27097/
Test FAILed.

AmplabJenkins · 2020-06-12T23:23:23Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27098/
Test FAILed.

rkooo567 · 2020-06-12T23:32:49Z

@ijrsvt Can you write some integration tests?

rkooo567 · 2020-06-12T23:33:43Z

python/ray/node.py

+                logger.error(
+                    "Internal Config parameters can only be set on the head "
+                    "node. NOTE, this include lru_evict.")
+                self._config == str(dict())


Why not just raising errors?

Yeah, we should just be raising an error here.

Also, lru_evict isn't an internal config parameter from the user's perspective so we shouldn't mention it in the error message. The user should just know they can't pass internal_config=XXX when starting a non-head node.

But lru_evict does modify internal_config parameters--how should that be handled?

I'd just have a separate check for lru_evict flag - if it's set and not head raise an error that says lru_evict can only be set on head node

I just added it to the massive check block in worker.py

AmplabJenkins · 2020-06-12T23:37:53Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27099/
Test FAILed.

AmplabJenkins · 2020-06-12T23:45:05Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27100/
Test FAILed.

simon-mo · 2020-06-13T04:46:23Z

Can you add some description to this PR

src/ray/gcs/accessor.h

ffbin · 2020-06-13T12:51:54Z

src/ray/gcs/gcs_server/gcs_table_storage.h

  }
 };

+class GcsInternalConfigTable : public GcsTable<UniqueID, StoredConfig> {


How about create a kV table? Both key and value type are string. There are other scenarios that need to use it, such as saving GCS server address. The GcsKVTable can use StoreClient directly.

This is probably a good idea but not sure it should be a blocker for this PR

edoakes

Nice work! Comments are basically all about style, nothing too major.

edoakes · 2020-06-15T15:08:20Z

python/ray/node.py

+                logger.error(
+                    "Internal Config parameters can only be set on the head "
+                    "node. NOTE, this include lru_evict.")
+                self._config == str(dict())


Yeah, we should just be raising an error here.

Also, lru_evict isn't an internal config parameter from the user's perspective so we shouldn't mention it in the error message. The user should just know they can't pass internal_config=XXX when starting a non-head node.

src/ray/gcs/accessor.h

src/ray/gcs/gcs_client/service_based_accessor.cc

src/ray/protobuf/gcs.proto

src/ray/raylet/main.cc

src/ray/raylet/raylet.h

Co-authored-by: Edward Oakes <ed.nmi.oakes@gmail.com>

…_config

ijrsvt · 2020-06-29T23:57:12Z

@rkooo567 I actually just added to test_multi_node_2.py::test_internal_config. Unless you want me to actually test that it is in GCS?

src/ray/gcs/gcs_client/service_based_accessor.cc

src/ray/gcs/accessor.h

src/ray/gcs/gcs_client/global_state_accessor.cc

AmplabJenkins · 2020-06-30T17:26:29Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27688/
Test FAILed.

…_config

AmplabJenkins · 2020-06-30T22:04:35Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27721/
Test FAILed.

AmplabJenkins · 2020-07-01T00:44:36Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27736/
Test FAILed.

src/ray/gcs/gcs_client/service_based_accessor.cc

ijrsvt · 2020-07-01T16:05:50Z

Thanks @kisuke95 for all the help!

AmplabJenkins · 2020-07-01T16:06:48Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27776/
Test FAILed.

kisuke95 · 2020-07-01T16:26:56Z

@ijrsvt My pleasure.

AmplabJenkins · 2020-07-02T01:27:50Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27801/
Test PASSed.

src/ray/gcs/gcs_client/service_based_accessor.cc

AmplabJenkins · 2020-07-02T03:09:41Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27796/
Test PASSed.

AmplabJenkins · 2020-07-02T07:57:24Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27817/
Test PASSed.

AmplabJenkins · 2020-07-06T20:33:55Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27915/
Test FAILed.

…_config

AmplabJenkins · 2020-07-08T04:28:32Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27994/
Test FAILed.

ijrsvt added 6 commits June 12, 2020 10:58

WIP

35d383f

lint

750d2b5

working setup

3d9750f

error if config set

4b0f93b

cleaning up tests

7293a2b

format fix

6321fdc

Cleaning up

b3c4627

Merge branch 'master' into internal_config

fbded52

ijrsvt requested a review from edoakes June 12, 2020 23:20

rkooo567 requested a review from ffbin June 12, 2020 23:30

fixing proto files

fedaccd

rkooo567 self-assigned this Jun 12, 2020

rkooo567 removed their assignment Jun 12, 2020

rkooo567 reviewed Jun 12, 2020

View reviewed changes

More cleanup

bb16ffa

ffbin reviewed Jun 13, 2020

View reviewed changes

src/ray/gcs/accessor.h Outdated Show resolved Hide resolved

ffbin reviewed Jun 13, 2020

View reviewed changes

edoakes reviewed Jun 15, 2020

View reviewed changes

ijrsvt and others added 4 commits June 15, 2020 10:53

Update src/ray/gcs/accessor.h

e10cb0d

Co-authored-by: Edward Oakes <ed.nmi.oakes@gmail.com>

Apply eds suggestions

c34899b

Co-authored-by: Edward Oakes <ed.nmi.oakes@gmail.com>

Renaming & removing comments

bbaf5e5

Merge branch 'internal_config' of github.com:ijrsvt/ray into internal…

dc99e73

…_config

ijrsvt added 2 commits June 29, 2020 16:58

moved test to multi_node_2

06c70dd

Merge branch 'master' into internal_config

2230404

kisuke95 requested changes Jun 30, 2020

View reviewed changes

ijrsvt added 2 commits June 30, 2020 14:06

Respond to comments

8f94fd4

Merge branch 'internal_config' of github.com:ijrsvt/ray into internal…

be210f4

…_config

build fix

216946e

kisuke95 requested changes Jul 1, 2020

View reviewed changes

src/ray/gcs/gcs_client/service_based_accessor.cc Outdated Show resolved Hide resolved

Changed Service based accessor insert

7e49469

kisuke95 approved these changes Jul 1, 2020

View reviewed changes

ijrsvt added 2 commits July 1, 2020 13:55

Try to fix bazel build issue

f2160b3

Style fix

88d4e2b

kisuke95 requested changes Jul 2, 2020

View reviewed changes

src/ray/gcs/gcs_client/service_based_accessor.cc Show resolved Hide resolved

further fixes

944febb

Merge branch 'master' into internal_config

2116bda

ijrsvt added 2 commits July 7, 2020 12:06

fixing final tests

487d023

Merge branch 'internal_config' of github.com:ijrsvt/ray into internal…

b35af18

…_config

edoakes merged commit 9172f8c into ray-project:master Jul 8, 2020

ijrsvt deleted the internal_config branch August 27, 2020 04:20

Conversation

ijrsvt commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

AmplabJenkins commented Jun 12, 2020

Uh oh!

AmplabJenkins commented Jun 12, 2020

Uh oh!

AmplabJenkins commented Jun 12, 2020

Uh oh!

AmplabJenkins commented Jun 12, 2020

Uh oh!

rkooo567 commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkooo567 Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

edoakes Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijrsvt Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

edoakes Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

ijrsvt Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Jun 12, 2020

Uh oh!

AmplabJenkins commented Jun 12, 2020

Uh oh!

simon-mo commented Jun 13, 2020

Uh oh!

Uh oh!

ffbin Jun 13, 2020

Choose a reason for hiding this comment

Uh oh!

edoakes Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

edoakes left a comment

Choose a reason for hiding this comment

Uh oh!

edoakes Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ijrsvt commented Jun 29, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AmplabJenkins commented Jun 30, 2020

Uh oh!

AmplabJenkins commented Jun 30, 2020

Uh oh!

AmplabJenkins commented Jul 1, 2020

Uh oh!

Uh oh!

ijrsvt commented Jul 1, 2020

Uh oh!

AmplabJenkins commented Jul 1, 2020

ijrsvt commented Jun 12, 2020 •

edited

Loading

rkooo567 commented Jun 12, 2020 •

edited

Loading

edoakes Jun 15, 2020 •

edited

Loading

edoakes Jun 15, 2020 •

edited

Loading