Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plasma C extensions #34

Merged
merged 3 commits into from
Nov 14, 2016
Merged

Plasma C extensions #34

merged 3 commits into from
Nov 14, 2016

Conversation

pcmoritz
Copy link
Contributor

No description provided.

@pcmoritz pcmoritz changed the title Numbuf Plasma C extensions Nov 11, 2016
plasma_get(conn, object_id, &size, &data, &metadata_size, &metadata);
PyObject *t = PyTuple_New(2);
PyTuple_SetItem(t, 0, PyBuffer_FromReadWriteMemory((void *) data, size));
PyTuple_SetItem(t, 1, PyBuffer_FromReadWriteMemory((void *) metadata, metadata_size));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably want PyBuffer_FromMemory, right?

@pcmoritz pcmoritz force-pushed the numbuf branch 8 times, most recently from c0b8c09 to 00c0dc0 Compare November 13, 2016 19:50
popd
cp "$PLASMA_DIR/build/plasma_store" "$PYTHON_PLASMA_DIR/build/"
cp "$PLASMA_DIR/build/plasma_manager" "$PYTHON_PLASMA_DIR/build/"
cp "$PLASMA_DIR/build/plasma_client.so" "$PYTHON_PLASMA_DIR/build/"
cp "$PLASMA_DIR/lib/python/plasma.py" "$PYTHON_PLASMA_DIR/lib/python/"
cp "$PLASMA_DIR/build/libplasma.so" "$PYTHON_PLASMA_DIR/lib/python"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the directory structure should match, so this should either be

cp "$PLASMA_DIR/build/libplasma.so" "$PYTHON_PLASMA_DIR/build/"

or

cp "$PLASMA_DIR/lib/python/libplasma.so" "$PYTHON_PLASMA_DIR/lib/python/"

@@ -37,4 +37,4 @@ elif [[ $platform == "macosx" ]]; then
fi

sudo pip install --upgrade git+git://github.com/cloudpipe/cloudpickle.git@0d225a4695f1f65ae1cbb2e0bbc145e10167cce4 # We use the latest version of cloudpickle because it can serialize named tuples.
sudo pip install --upgrade --verbose git+git://github.com/ray-project/numbuf.git@d1974afbab9f0f1bcf8af15a8c476d868ad31aff
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you change this here, you should also change it in install-on-macos.md and install-on-ubuntu.md.


def random_string():
return "".join([chr(random.randint(0, 255)) for _ in range(20)])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Purely aesthetic, but I think it'd be better to define random_string before random_object_id since random_object_id uses random_string.

pushd "$PLASMA_DIR/build"
cmake ..
make install
popd
popd
cp "$PLASMA_DIR/build/plasma_store" "$PYTHON_PLASMA_DIR/build/"
cp "$PLASMA_DIR/build/plasma_manager" "$PYTHON_PLASMA_DIR/build/"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we no longer need to build plasma_client.so, right? Let's remove all references to that.

def wait(self, object_ids, timeout, num_returns):
return libplasma.fetch(self.conn, object_ids)

def wait(self, object_ids, timeout=2**32-1, num_returns=1):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace, just save a variable timeout = 2 ** 32 - 1 and then timeout=timeout.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that this is a bug. The timeout should be a 64 bit integer. But that doesn't work for some reason.

int port,
object_id object_id);

/**
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't this already here? If not, how were we doing the transfer tests before?

(plasma_connection *) PyCapsule_GetPointer(capsule, "plasma");
plasma_disconnect(conn);
*/
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason we can't implement this now?

#include "common.h"
#include "plasma_client.h"

static int PyObjectToPlasma(PyObject *object, plasma_connection **conn) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be PyObjectToPlasmaConnection, right?

PyErr_SetString(
PyExc_RuntimeError,
"The argument num_returns cannot be greater than len(object_ids)");
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to return NULL here, right?

@@ -91,8 +91,12 @@ struct plasma_store_state {
plasma_store_info *plasma_store_info;
/** The state that is managed by the eviction policy. */
eviction_state *eviction_state;
/** Input buffer. */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please document the purpose of this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants