Use strongly typed IDs in C++. #4185

guoyuhong · 2019-02-27T16:01:51Z

What do these changes do?

Use strongly typed IDs in C++. Now, the compiler will complain about misusing of ids. The user needs to call the explicit constructor to cast the type.
Update cython part to adapt the c++ changes.

Related issue number

AmplabJenkins · 2019-02-27T18:43:21Z

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

pcmoritz · 2019-02-27T22:27:22Z

python/ray/includes/unique_ids.pxi

Do we really want to allocate this on the heap? Seems like for such a small object it would be better to do it inline, can that be made to work?

Yes, I removed all the heap allocations.

pcmoritz · 2019-02-27T22:28:14Z

The C++ changes are great, for the cython ones, is there a way to get rid of the heap allocations (and also the code duplications if possible).

robertnishihara · 2019-02-28T08:36:24Z

This is a huge improvement. I've had plenty of bugs in the past where I mixed up the ID types (e.g., passed ID arguments into a function in the wrong order) that would have been caught by this.

robertnishihara · 2019-02-28T08:36:52Z

This should fix #3721, right?

guoyuhong · 2019-02-28T09:08:37Z

@robertnishihara Yes, that will fix #3721 .

AmplabJenkins · 2019-02-28T12:35:05Z

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

AmplabJenkins · 2019-02-28T13:30:53Z

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

AmplabJenkins · 2019-02-28T14:00: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/12390/
Test FAILed.

guoyuhong · 2019-02-28T15:35:50Z

@AmplabJenkins retest this, please.

AmplabJenkins · 2019-02-28T18:42:33Z

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

guoyuhong · 2019-03-01T03:22:16Z

@AmplabJenkins retest this, please.

AmplabJenkins · 2019-03-01T05:22: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/12403/
Test FAILed.

pcmoritz · 2019-03-01T06:14:39Z

python/ray/includes/unique_ids.pxi

Can this be invoked in a situation where self is not of type ObjectID?

Maybe this is only useful in __cinit__ and it is not necessary in __init__. I will do a double confirm. For __cinit__, the functions of derived class and base class will both be called, so using this if statement will avoid duplicated initiation.

@pcmoritz In cython, __init__ behaves the same as python's. This function will be called only once. On the contrast, __cinit__ will be called in both base class and derived class. I have removed the type check. Thanks!

AmplabJenkins · 2019-03-01T13:01:26Z

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

AmplabJenkins · 2019-03-02T12:27:27Z

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

AmplabJenkins · 2019-03-03T11:51:18Z

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

AmplabJenkins · 2019-03-04T11:42: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/12526/
Test FAILed.

raulchen · 2019-03-04T12:09:06Z

python/ray/includes/unique_ids.pxd

it seems that these constructors aren't used in Cython. We can remove them to minimize redundant code.

I removed both constructors for each cppclass.

AmplabJenkins · 2019-03-06T12:42:06Z

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

guoyuhong · 2019-03-07T05:24:15Z

Do you have more comments? I rebased the PR every day avoid logic conflicts with other PRs.

pcmoritz

This is great!

AmplabJenkins · 2019-03-07T09:58:11Z

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

pcmoritz reviewed Feb 27, 2019

View reviewed changes

guoyuhong changed the title ~~Use strongly typed IDs for C++.~~ Use strongly typed IDs in C++. Feb 28, 2019

guoyuhong force-pushed the strongTypeID branch 2 times, most recently from 178fcba to a880709 Compare February 28, 2019 10:53

pcmoritz reviewed Mar 1, 2019

View reviewed changes

guoyuhong force-pushed the strongTypeID branch from a880709 to 6207e90 Compare March 1, 2019 10:25

guoyuhong force-pushed the strongTypeID branch from 6207e90 to 2a71d37 Compare March 2, 2019 09:55

guoyuhong force-pushed the strongTypeID branch from 2a71d37 to 31202ee Compare March 3, 2019 09:14

guoyuhong force-pushed the strongTypeID branch from 31202ee to 53e841f Compare March 4, 2019 08:41

raulchen reviewed Mar 4, 2019

View reviewed changes

robertnishihara assigned pcmoritz Mar 6, 2019

guoyuhong force-pushed the strongTypeID branch from 53e841f to 65f15de Compare March 6, 2019 09:35

Use strongly typed IDs for C++.

6889b95

Yuhong Guo added 6 commits March 7, 2019 11:50

Avoid heap allocation in cython.

fbf26cc

Fix JNI part

aa19739

Fix rebase conflict

d3c1eae

Refine

b79d4e2

Remove type check from __init__

204b6b1

Remove unused constructor declarations.

c86c041

guoyuhong force-pushed the strongTypeID branch from 65f15de to c86c041 Compare March 7, 2019 05:22

pcmoritz approved these changes Mar 7, 2019

View reviewed changes

guoyuhong merged commit b9ea821 into ray-project:master Mar 7, 2019

guoyuhong deleted the strongTypeID branch March 7, 2019 13:43

robertnishihara mentioned this pull request Mar 12, 2019

ID types should be made type safe. #3721

Closed

Use strongly typed IDs in C++. #4185

Use strongly typed IDs in C++. #4185

Uh oh!

Conversation

guoyuhong commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do?

Related issue number

Uh oh!

AmplabJenkins commented Feb 27, 2019

Uh oh!

pcmoritz Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

guoyuhong Feb 28, 2019

Choose a reason for hiding this comment

Uh oh!

pcmoritz commented Feb 27, 2019

Uh oh!

robertnishihara commented Feb 28, 2019

Uh oh!

robertnishihara commented Feb 28, 2019

Uh oh!

guoyuhong commented Feb 28, 2019

Uh oh!

AmplabJenkins commented Feb 28, 2019

Uh oh!

AmplabJenkins commented Feb 28, 2019

Uh oh!

AmplabJenkins commented Feb 28, 2019

Uh oh!

guoyuhong commented Feb 28, 2019

Uh oh!

AmplabJenkins commented Feb 28, 2019

Uh oh!

guoyuhong commented Mar 1, 2019

Uh oh!

AmplabJenkins commented Mar 1, 2019

Uh oh!

pcmoritz Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

guoyuhong Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

guoyuhong Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Mar 1, 2019

Uh oh!

AmplabJenkins commented Mar 2, 2019

Uh oh!

AmplabJenkins commented Mar 3, 2019

Uh oh!

AmplabJenkins commented Mar 4, 2019

Uh oh!

raulchen Mar 4, 2019

Choose a reason for hiding this comment

Uh oh!

guoyuhong Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Mar 6, 2019

Uh oh!

guoyuhong commented Mar 7, 2019

Uh oh!

pcmoritz left a comment

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Mar 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

guoyuhong commented Feb 27, 2019 •

edited

Loading