Migrate Python C extension to Cython #3541

suquark · 2018-12-14T05:26:25Z

What do these changes do?

This PR migrates Python C extension to Cython bindings for the raylet client.

Related issue number

#3474
This also fixes #405

AmplabJenkins · 2018-12-14T07:33: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/10042/
Test FAILed.

AmplabJenkins · 2019-01-10T17:33:02Z

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

raulchen · 2019-01-11T06:19:13Z

python/ray/includes/unique_ids.pxi

Not necessary to duplicate code for each id type. We can define one base class and have all other classes extend it. In the super class, we can leverage cls and self.__class__ to get the concrete type. Here is a short example.

class UniqueID(object): def __init__(self, data): self.data = data @classmethod def from_random(cls): return cls(b'') def __eq__(self, other): return self.__class__ == other.__class__ and self.data == other.data class TaskID(UniqueID): pass class ObjectID(UniqueID): pass id1 = TaskID(b'') id2 = TaskID.from_random() id3 = ObjectID(b'') assert type(id1) == TaskID assert type(id2) == TaskID assert type(id3) == ObjectID assert id1 == id2 assert id1 != id3

Also, it's worth noticing that this is not real type safe. Because types in Python are dynamic. For example, if I have a function that is supposed to take a TaskID parameter, I can still pass in an ObjectID.

One reason I am defining those IDs as independent classes is because those C++ types could be strong-typedefed in the near future.
I have also tried to reuse some duplicated parts. Unfortunately, Cython cannot recognize classes from cls or __class__ in many cases. So I still need to write a lot of extra code.

Do you see any other way to get rid of the code duplication? It would be good to resolve this before the PR is merged.

I agree that all of these types will be different C++ classes in the future, so we should plan for that setting. It's also possible that they will have different methods, e.g, with ObjectID you could potentially call GetTaskID() or IsPut(), but that wouldn't make sense with an ActorID.

Agreed, also this of course doesn't mean they can't share code. I'm pretty sure with interitence it can be made to work well that the common methods are defined only once, I'll give it a shot.

AmplabJenkins · 2019-01-12T14:16:31Z

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

suquark · 2019-01-13T17:04:48Z

I have implemented all features in Cython. Some code still needs to be updated to fit the Cython interface.

AmplabJenkins · 2019-01-13T17:56:52Z

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

AmplabJenkins · 2019-01-14T11:52: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/10840/
Test FAILed.

AmplabJenkins · 2019-01-14T12:08:43Z

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

…-project#3541

* convert code to proper C++ * revert changes to "id.h" because #3765 has been merged. * revert changes to Python bindings because they will be removed in #3541 * remove dependencies of Arrow logging * revert changes to Arrow logging * lint

pcmoritz · 2019-01-20T04:21:16Z

python/ray/function_manager.py

-        function_id = ray.ObjectID(function_id_str)
-        driver_id = ray.ObjectID(driver_id_str)
+        function_id = ray.FunctionID(function_id_str)
+        # TODO(suquark): Make sure the type of "driver_id".


This is the right type, do you want to remove the TODO?

JobID and DriverID are the same, and in the future I'd say we should transition to using DriverID exclusively. But given that in C++ it's called JobID, I think it's ok to call it JobID in python too until we change it everywhere.

Yeah, we actually use both DriverID and JobID in C++. I agree about using only one (e.g., DriverID).

pcmoritz · 2019-01-20T04:21:47Z

python/ray/function_manager.py

        class_name = decode(class_name)
        module = decode(module)
-        driver_id = ray.ObjectID(driver_id_str)
+        # TODO(suquark): Ensure the type of "driver_id".


same as above

pcmoritz · 2019-01-20T04:22:49Z

python/ray/includes/common.pxd

+from libcpp.string cimport string as c_string
+from libcpp cimport bool as c_bool
+
+from libc.stdint cimport int64_t as c_int64, uint32_t as c_uint32, uint16_t as c_uint16, uint8_t as c_uint8


Is there any clashes or should we just import these as int64_t etc?

pcmoritz · 2019-01-20T04:24:07Z

python/ray/includes/common.pxd

+cdef extern from "flatbuffers/flatbuffers.h" namespace "flatbuffers":
+    # Cython cannot rename a template class.
+    cdef cppclass Offset[T]:
+        uoffset_t o


is this needed? Shall we call it offset instead?

AmplabJenkins · 2019-01-23T12:12:40Z

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

src/ray/ray_config.h

AmplabJenkins · 2019-01-23T13:31:00Z

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

AmplabJenkins · 2019-01-23T20:43:58Z

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

AmplabJenkins · 2019-01-24T00:57:42Z

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

AmplabJenkins · 2019-01-24T01:14:10Z

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

AmplabJenkins · 2019-01-24T01:50:54Z

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

AmplabJenkins · 2019-01-24T04:14:46Z

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

AmplabJenkins · 2019-01-24T05:31:13Z

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

AmplabJenkins · 2019-01-24T07:22:43Z

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

AmplabJenkins · 2019-01-24T08:13:26Z

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

AmplabJenkins · 2019-01-24T08:26:07Z

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

AmplabJenkins · 2019-01-24T08:31:20Z

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

AmplabJenkins · 2019-01-24T16:33:40Z

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

suquark · 2019-01-24T17:00:07Z

Ready to merge. The CI errors come from the master branch.

suquark self-assigned this Jan 8, 2019

pcmoritz mentioned this pull request Jan 10, 2019

Get rid of Arrow test utils #3734

Merged

suquark force-pushed the interface branch from 7113758 to 711328e Compare January 10, 2019 15:28

suquark mentioned this pull request Jan 11, 2019

Refactor code about ray.ObjectID. #3674

Merged

raulchen reviewed Jan 11, 2019

View reviewed changes

suquark force-pushed the interface branch from 711328e to f6b056c Compare January 12, 2019 14:01

suquark force-pushed the interface branch from a2bb703 to 953cdf5 Compare January 13, 2019 16:55

suquark added 8 commits January 14, 2019 19:16

initial commit for cython bindings

a788bd1

revert changes caused by pycharm formatter

5ed340e

update cmake

9b0ea7a

fulfill cython bindings

03b220c

fix python bindings

c7b5312

bind Task & RayConfig. Fix CMake settings.

4537993

fully implemented Python bindings

9f404f3

Use strong-typed IDs. Bug fix.

9516f35

suquark force-pushed the interface branch from 953cdf5 to 9516f35 Compare January 14, 2019 11:17

fix actor

0ed790b

suquark added a commit to suquark/ray that referenced this pull request Jan 14, 2019

revert changes to Python bindings because they will be removed in ray…

370e0ec

…-project#3541

pcmoritz reviewed Jan 20, 2019

View reviewed changes

raulchen reviewed Jan 23, 2019

View reviewed changes

src/ray/ray_config.h Outdated Show resolved Hide resolved

raulchen mentioned this pull request Jan 23, 2019

Remove duplicated code in RayConfig #3831

Merged

pcmoritz force-pushed the interface branch from 772c406 to a78de7f Compare January 24, 2019 05:19

pcmoritz force-pushed the interface branch 4 times, most recently from 141bc91 to 308985c Compare January 24, 2019 05:57

some small fixes

7bd4f60

pcmoritz force-pushed the interface branch from 308985c to 7bd4f60 Compare January 24, 2019 05:58

merge upstream/master

28d2b27

pcmoritz approved these changes Jan 24, 2019

View reviewed changes

pcmoritz merged commit 48139cf into ray-project:master Jan 24, 2019

robertnishihara mentioned this pull request Jan 24, 2019

Migrate Python C extension to Cython? #3474

Closed

suquark deleted the interface branch January 25, 2019 01:06

robertnishihara mentioned this pull request Jan 25, 2019

Too many warning messages when actor handles are passed into tasks. #3850

Closed

robertnishihara mentioned this pull request Feb 6, 2019

ObjectIDs should be referenceable in closures #1453

Closed

Migrate Python C extension to Cython #3541

Migrate Python C extension to Cython #3541

Uh oh!

Conversation

suquark commented Dec 14, 2018 • 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 Dec 14, 2018

Uh oh!

AmplabJenkins commented Jan 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcmoritz Jan 20, 2019 • 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!

AmplabJenkins commented Jan 12, 2019

Uh oh!

suquark commented Jan 13, 2019

Uh oh!

AmplabJenkins commented Jan 13, 2019

Uh oh!

AmplabJenkins commented Jan 14, 2019

Uh oh!

AmplabJenkins commented Jan 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Jan 23, 2019

Uh oh!

Uh oh!

AmplabJenkins commented Jan 23, 2019

Uh oh!

AmplabJenkins commented Jan 23, 2019

Uh oh!

AmplabJenkins commented Jan 24, 2019

Uh oh!

AmplabJenkins commented Jan 24, 2019

Uh oh!

AmplabJenkins commented Jan 24, 2019

Uh oh!

AmplabJenkins commented Jan 24, 2019

Uh oh!

AmplabJenkins commented Jan 24, 2019

Uh oh!

AmplabJenkins commented Jan 24, 2019

Uh oh!

AmplabJenkins commented Jan 24, 2019

Uh oh!

AmplabJenkins commented Jan 24, 2019

Uh oh!

AmplabJenkins commented Jan 24, 2019

Uh oh!

AmplabJenkins commented Jan 24, 2019

Uh oh!

suquark commented Jan 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

suquark commented Dec 14, 2018 •

edited

Loading

pcmoritz Jan 20, 2019 •

edited

Loading