ARROW-1149: [Plasma] Create Cython client library for Plasma by pcmoritz · Pull Request #797 · apache/arrow

pcmoritz · 2017-06-30T01:31:33Z

This PR introduces a Cython API to Plasma, a FindPlasma.cmake to make it easier to integrate Plasma with CMake projects and sets up packaging with pyarrow.

wesm · 2017-07-01T01:01:26Z

@pcmoritz I'm sorry about the delay, I will have a closer look most likely tomorrow

pcmoritz · 2017-07-01T02:24:07Z

Thanks, looking forward to it! The central question is how much mixing between the plasma client and pyarrow we want and also how the directory structure should look like. I kept them pretty separate for now.

wesm

Thanks for doing this! As far as the packaging, it's a little more work to add a FindPlasma.cmake file to the pyarrow build system, but for packaging simplicity (for pip and conda) it would be easiest to maintain this as pyarrow/plasma.pyx and have a single build system and package artifact

I left minor comments about proper usage of nogil (you have to have the with nogil: block for it to release the GIL; the nogil annotation in the cdef extern blocks tells Cython that the functions are safe to use in a nogil block, but the actual release must be explicit)

The other thing is the docstrings so that things will render properly when we add these APIs to the Arrow Python documentation (http://arrow.apache.org/docs/python/api.html -- it's in need of some love as you can see)

wesm · 2017-07-02T20:29:16Z

+#ifdef PyMODINIT_FUNC
+#undef PyMODINIT_FUNC
+#endif
+#define PyMODINIT_FUNC extern "C" __attribute__((visibility ("default"))) PyObject*


This is very odd, will have to have a closer look at what is going on; I've never had any issues like this

wesm · 2017-07-02T20:29:59Z

+
+cdef extern from "plasma/common.h" nogil:
+
+  cdef cppclass CUniqueID" plasma::UniqueID":


Mixed indentation -- use 4 spaces everywhere?

wesm · 2017-07-02T20:31:12Z

+    self.data = CUniqueID.from_binary(object_id)
+
+  def __richcmp__(ObjectID self, ObjectID object_id, operation):
+    assert operation == 2, "only equality implemented so far"


Raise a ValueError here instead

wesm · 2017-07-02T20:32:46Z

+    for object_id in object_ids:
+      ids.push_back(object_id.data)
+    result[0].resize(ids.size())
+    check_status(self.client.get().Get(ids.data(), ids.size(), timeout_ms, result[0].data()))


Add with nogil: here to release the GIL

wesm · 2017-07-02T20:33:34Z

+      release_delay (int): The maximum number of objects that the client will
+        keep and delay releasing (for caching reasons).
+    """
+    check_status(self.client.get().Connect(store_socket_name.encode(), manager_socket_name.encode(), release_delay))


with nogil: here (may require some unboxing of the Python objects outside the with nogil block)

wesm · 2017-07-02T20:35:43Z

+
+  def release(self, ObjectID object_id):
+    """Notify Plasma that the object is no longer needed."""
+    check_status(self.client.get().Release(object_id.data))


wesm · 2017-07-02T20:35:49Z

+      object_id (str): A string used to identify an object.
+    """
+    cdef c_bool is_contained
+    check_status(self.client.get().Contains(object_id.data, &is_contained))


wesm · 2017-07-02T20:35:57Z

+      store, the string will have length zero.
+    """
+    cdef c_vector[uint8_t] digest = c_vector[uint8_t](kDigestSize)
+    check_status(self.client.get().Hash(object_id.data, digest.data()))


wesm · 2017-07-02T20:36:08Z

+      num_bytes (int): The number of bytes to attempt to recover.
+    """
+    cdef int64_t num_bytes_evicted = -1
+    check_status(self.client.get().Evict(num_bytes, num_bytes_evicted))


with nogil, and the other client calls below

wesm · 2017-07-02T20:37:16Z

+
+USE_VALGRIND = False
+
+def random_name():


Indent with 4 spaces in this file

pcmoritz · 2017-07-04T06:01:08Z

Thanks a lot for the review. The packaging simplicity is a good argument, let's make it part of pyarrow. I'm traveling right now but hope to get to it this weekend.

robertnishihara · 2017-07-04T06:12:25Z

+}
+
+# run tests for python 2.7 and 3.6
+# python_version_tests 2.7


probably comment this in?

robertnishihara · 2017-07-04T06:14:48Z

        read_multiple_files(mixed_paths)

-
+@parquet


probably need an extra newline before @parquet

xhochy · 2017-07-14T13:43:25Z

@pcmoritz please ping me when I should review or you run into problems. I'm ignoring this pull request for now as there's a lot of activity in here with commits.

pcmoritz · 2017-07-14T13:53:04Z

@xhochy I'm wrapping it up right now, the last thing I'm really blocked on is the error in https://travis-ci.org/pcmoritz/arrow/jobs/253597521 in the manylinux build, see the error message
"ImportError: libplasma.so.0: cannot open shared object file: No such file or directory"

If you have an idea please let me know! I did add -DARROW_PLASMA=ON to python/manylinux1/build_arrow.sh and FindPlasma.cmake seems to find libplasma.so at least.

Other than that there are some doc fixes I need to do, I'll let you know when it is ready.

Thanks for your help :)

robertnishihara · 2017-07-14T15:21:36Z

I'm a little concerned about not also testing this on OSX, but I agree it might be too burdensome on Travis.

added tests for mac os now, if it becomes too burdensome, we can switch it off

robertnishihara · 2017-07-14T15:50:39Z

two newlines before and after this class

robertnishihara · 2017-07-14T16:11:16Z

In Ray, this used to look more like

if self.plasma_client.alive: libplasma.release(self.plasma_client.conn, self.plasma_id)

I think the motivation for this was that Ray (on a single machine) by default kills the plasma store manually at the end of a script. After this, a PlasmaBuffer can still go out of scope and try to send a release message to the store, and the check_status will raise an exception leaving some ugly error or warning messages.

Or is this avoided in some other way?

This is now done in the C++ client; the file descriptor is set to -1 on disconnect, our old shutdown is disconnect and release checks for the file descriptor being -1.

I see, that looks good.

xhochy · 2017-07-16T11:00:57Z

We could also move these cmake_modules to the top-level so that they are available to any arrow-cpp component.

Yeah, I think that's where they belong. Shall we do this as a separate PR to make it a little more visible? It might break other people's scripts.

xhochy · 2017-07-16T11:05:11Z

Can you add a check in line 67 below that plasma can be imported?

Yes, thanks for the pointer!

xhochy · 2017-07-16T11:06:41Z

Please specify valgrind and plasma options as part of the pytest call, see the --parquet option in pyarrow/tests/conftest.py

pcmoritz · 2017-07-19T20:33:13Z

The build is now green and it's ready to merge. One possible reservation is that travis times are quite long now, any thoughts on that?

wesm · 2017-07-23T18:52:23Z

I will give this a last look and merge later today assuming no issues now that the 0.5.0 release vote has passed. I will also look at the build times to see if there's anything that needs to be done

pcmoritz · 2017-07-23T19:04:50Z

Great, thanks! Next steps from our side is a little bit more high level documentation (I'll create a PR once this is merged) and then the blog post.

wesm · 2017-07-23T23:11:13Z

Sounds good. We can probably publish the blog post right after the IP clearance process is concluded

wesm · 2017-07-24T01:42:07Z

There's a significant amount of redundant work happening in the build. We should probably only be testing the built-in third-party toolchain (the ExternalProjects) in one entry in the build matrix and using toolchain libraries in all the others. This would probably be simplest to tackle in a follow up patch. I can take a look

wesm

Minor comments -- sorry to delay getting this in, but I think we should remove the MutableBuffer classes and instead have a mutable property that forwards buffer->is_mutable(). I can look into speeding up the builds in a separate patch

wesm · 2017-07-24T01:49:08Z

+   ObjectID
+   PlasmaClient
+   PlasmaBuffer
+   MutablePlasmaBufferMutablePlasmaBuffer


Typo. Can address when working on documentation generally

wesm · 2017-07-24T01:49:40Z

        int64_t GetExtentBytesWritten()

+    cdef cppclass CFixedSizeBufferWrite" arrow::io::FixedSizeBufferWriter"(WriteableFile):
+        CFixedSizeBufferWrite(const shared_ptr[CBuffer]& buffer)


wesm · 2017-07-24T01:51:31Z

        return self.size


+cdef class MutableBuffer(Buffer):


Since buffers have an is_mutable() function, is this needed (versus adding a mutable property to Buffer)?

wesm · 2017-07-24T01:52:21Z

+        self.wr_file.reset(new CFixedSizeBufferWrite(buffer.buffer))
+        self.is_readable = 0
+        self.is_writeable = 1
+        self.is_open = True


May want to move this to io.pxi

wesm · 2017-07-24T01:53:26Z

+        self.client.release(self.object_id)
+
+
+cdef class MutablePlasmaBuffer(MutableBuffer):


Let's see if this can be collapsed per the discussion above with MutableBuffer

wesm · 2017-07-24T01:55:44Z

+            use_valgrind=os.getenv("PLASMA_VALGRIND") == "1")
+        # Connect to Plasma.
+        self.plasma_client = plasma.PlasmaClient()
+        self.plasma_client.connect(plasma_store_name, "", 64)


What do you think about adding a top level function pyarrow.plasma.connect that returns a fully-baked client? This API is a slight rough edge

Yeah, you are right, that seems better!

pcmoritz · 2017-07-24T06:36:21Z

        self.with_plasma = strtobool(
            os.environ.get('PYARROW_WITH_PLASMA', '0'))
-        if self.with_plasma:
+        if self.with_plasma and "plasma" not in self.CYTHON_MODULE_NAMES:


The function initialize_options seems to have been called twice and plasma was added twice to CYTHON_MODULE_NAMES, which broke the "python setup.py develop". If there is a better fix let me know!

pcmoritz · 2017-07-24T06:36:40Z

    def _failure_permitted(self, name):
        if name == '_parquet' and not self.with_parquet:
            return True
+        if name == 'plasma' and not self.with_plasma:


I added this for good measure, not sure where/if it is necessary

wesm · 2017-07-24T16:10:47Z

        c_string manager_socket_name

-    def __cinit__(self):
+    def __cinit__(self, store_socket_name, manager_socket_name, int release_delay):


As one rough edge in Cython, if you call the ctor with the wrong parameters you will generally segfault, which is why factory methods are usually safer. We can tackle this in a follow up patch

wesm

+1, thanks @pcmoritz!!

pcmoritz · 2017-07-24T18:41:07Z

Thanks for merging! If you could look into the ExternalProjects build times that'd be great (I'm not sure what is involved there). I can see if we can make the actual plasma tests a little faster (and can also change the PlasmaClient -> connect API).

pcmoritz force-pushed the plasma-cython branch from 6f7dae4 to 0832435 Compare June 30, 2017 04:50

wesm reviewed Jul 2, 2017

View reviewed changes

robertnishihara reviewed Jul 4, 2017

View reviewed changes

pcmoritz force-pushed the plasma-cython branch 7 times, most recently from abc5600 to 48b6f90 Compare July 14, 2017 12:36

robertnishihara reviewed Jul 14, 2017

View reviewed changes

robertnishihara mentioned this pull request Jul 14, 2017

Move plasma object store into Arrow. ray-project/ray#611

Closed

pcmoritz force-pushed the plasma-cython branch 2 times, most recently from 6f8ed1c to 0c65b4c Compare July 16, 2017 00:34

xhochy mentioned this pull request Jul 16, 2017

ARROW-1219: [C++] Use Google C++ code formatting #848

Closed

xhochy requested changes Jul 16, 2017

View reviewed changes

pcmoritz force-pushed the plasma-cython branch 8 times, most recently from 8f7b360 to dc3fecb Compare July 19, 2017 07:38

pcmoritz added 5 commits July 19, 2017 10:08

fix

997de1e

fix

b863d13

make plasma store binary part of the pyarrow package for tests

23fe5f5

debug

0bea267

fix segfault

3e4a84d

pcmoritz force-pushed the plasma-cython branch 4 times, most recently from de14fa9 to 1609f48 Compare July 19, 2017 18:41

fix windows build

b9e2dee

pcmoritz force-pushed the plasma-cython branch from 1609f48 to b9e2dee Compare July 19, 2017 19:01

changes needed to make Ray work with Plasma in Arrow

5f7b779

pcmoritz mentioned this pull request Jul 22, 2017

Pull Plasma from Apache Arrow and remove Plasma store from Ray. ray-project/ray#692

Merged

wesm reviewed Jul 24, 2017

View reviewed changes

fix setup.py develop for plasma

e33443d

pcmoritz commented Jul 24, 2017

View reviewed changes

pcmoritz added 3 commits July 23, 2017 23:50

fix typos and move FixedSizeBufferOutputStream

08f24a5

get rid of MutableBuffer

d14ab87

get for of PlasmaClient.connect

d8319fc

wesm reviewed Jul 24, 2017

View reviewed changes

wesm approved these changes Jul 24, 2017

View reviewed changes

asfgit closed this in a94f471 Jul 24, 2017


		cdef extern from "plasma/common.h" nogil:

		cdef cppclass CUniqueID" plasma::UniqueID":

		self.client.release(self.object_id)


		cdef class MutablePlasmaBuffer(MutableBuffer):

		read_multiple_files(mixed_paths)


		@parquet

Conversation

pcmoritz commented Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesm commented Jul 1, 2017

Uh oh!

pcmoritz commented Jul 1, 2017

Uh oh!

wesm left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

pcmoritz commented Jul 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xhochy commented Jul 14, 2017

Uh oh!

pcmoritz commented Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

pcmoritz commented Jul 19, 2017

Uh oh!

wesm commented Jul 23, 2017

Uh oh!

pcmoritz commented Jul 23, 2017

Uh oh!

wesm commented Jul 23, 2017

Uh oh!

wesm commented Jul 24, 2017

Uh oh!

wesm left a comment

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

pcmoritz commented Jun 30, 2017 •

edited

Loading

pcmoritz commented Jul 14, 2017 •

edited

Loading

pcmoritz Jul 24, 2017 •

edited

Loading