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

Add basic functionality for Cython functions and actors #1193

Merged
merged 6 commits into from
Nov 10, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ install:
- ./.travis/install-dependencies.sh
- export PATH="$HOME/miniconda/bin:$PATH"
- ./.travis/install-ray.sh
- ./.travis/install-cython-examples.sh

- cd python/ray/core
- bash ../../../src/common/test/run_tests.sh
Expand Down Expand Up @@ -120,6 +121,7 @@ script:
- python test/monitor_test.py
- python test/trial_runner_test.py
- python test/trial_scheduler_test.py
- python test/cython_test.py

- python -m pytest python/ray/rllib/test/test_catalog.py

Expand Down
38 changes: 38 additions & 0 deletions .travis/install-cython-examples.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#!/usr/bin/env bash

# Cause the script to exit if a single command fails
set -e

ROOT_DIR=$(cd "$(dirname "${BASH_SOURCE:-$0}")"; pwd)

echo "PYTHON is $PYTHON"

cython_examples="$ROOT_DIR/../examples/cython"

if [[ "$PYTHON" == "2.7" ]]; then

pushd $cython_examples
pip install scipy
python setup.py install --user
popd

elif [[ "$PYTHON" == "3.5" ]]; then
export PATH="$HOME/miniconda/bin:$PATH"

pushd $cython_examples
pip install scipy
python setup.py install --user
popd

elif [[ "$LINT" == "1" ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not install anything in the linting builds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, you're right.

export PATH="$HOME/miniconda/bin:$PATH"

pushd $cython_examples
pip install scipy
python setup.py install --user
popd

else
echo "Unrecognized Python version."
exit 1
fi
6 changes: 6 additions & 0 deletions examples/cython/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
build
Copy link
Collaborator

Choose a reason for hiding this comment

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

How do you feel about moving these to the top-level .gitignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing; I put in a local .gitignore since I didn't want to mess with the top-level one. Would it also be useful to put typical virtual environment directories? I waver on that one because even though there are some conventions (e.g., venv), it seems like folks have their own preferences / not totally standardized.

Copy link
Contributor Author

@danielsuo danielsuo Nov 9, 2017

Choose a reason for hiding this comment

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

I see that the ignores for dynamic libraries are already in the top-level .gitignore.

I think build is reasonable to go in top-level, but do we want *.c? The Cython convention is to ignore *.c files in repositories, but include them in distributions so users don't need to install cython to use the package. The concern is if, for some reason, we have C code. I put added *.c for now since we're in C++ / Python land.


*.c
*.so
*.dylib
*.dll
28 changes: 28 additions & 0 deletions examples/cython/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Cython in Ray
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be nice to include this in the online documentation.

That would require moving this to doc/source/ and linking to it from doc/source/index.rst.

That could be done as a follow up if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!


To get started, run the following from directory ```$RAY_HOME/examples/cython```:
bash
```
pip install scipy # For BLAS example
python setup.py develop
python cython_main.py --help
```

You can import the ```cython_examples``` module from a Python script or interpreter.

## Notes
- You **must** include the following two lines at the top of any ```*.pyx``` file:
```python
#!python
# cython: embedsignature=True, binding=True
```
- You cannot decorate Cython functions within a ```*.pyx``` file (there are ways around this, but creates a leaky abstraction between Cython and Python that would be very challenging to support generally). Instead, prefer the following in your Python code:
```
some_cython_func = ray.remote(some_cython_module.some_cython_func)
```
- You cannot transfer memory buffers to a remote function (see ```example8```, which currently fails); your remote function must return a value
- Have a look at ```cython_main.py```, ```cython_simple.pyx```, and ```setup.py``` for examples of how to call, define, and build Cython code, respectively. The Cython [documentation](http://cython.readthedocs.io/) is also very helpful.
- Several limitations come from Cython's own [unsupported](https://github.com/cython/cython/wiki/Unsupported) Python features.

## License
We include some code examples from the [BrainIAK](https://github.com/brainiak/brainiak) package. There are licenses associated with those examples, which may (or may not!) cause issue. We can remove prior to any merge.
Copy link
Contributor

Choose a reason for hiding this comment

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

As long as the code is Apache 2, which it seems to be, this should be ok!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since it's Apache 2, can we remove this section?

29 changes: 29 additions & 0 deletions examples/cython/cython_examples/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
from __future__ import absolute_import
Copy link
Collaborator

Choose a reason for hiding this comment

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

In all of the new files, can we use all three

from __future__ import absolute_import
from __future__ import division
from __future__ import print_function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I held off for the *.pyx files, but included an additional note in the documentation about Python 2 vs. 3 + where folks can get more information.


from .cython_simple import simple_func, fib, fib_int, \
fib_cpdef, fib_cdef, simple_class
from .masked_log import masked_log

from .cython_blas import \
compute_self_corr_for_voxel_sel, \
compute_kernel_matrix, \
compute_single_self_corr_syrk, \
compute_single_self_corr_gemm, \
compute_corr_vectors, \
compute_single_matrix_multiplication

__all__ = [
'simple_func',
'fib',
'fib_int',
'fib_cpdef',
'fib_cdef',
'simple_class',
'masked_log',
'compute_self_corr_for_voxel_sel',
'compute_kernel_matrix',
'compute_single_self_corr_syrk',
'compute_single_self_corr_gemm',
'compute_corr_vectors',
'compute_single_matrix_multiplication'
]
Loading