Add basic functionality for Cython functions and actors #1193

danielsuo · 2017-11-08T02:24:06Z

A first stab at addressing #944. Tests, including new ones for Cython, pass (besides timeouts) on my latest Travis build.

Some notes for reviewers:

Usage here
Pay special attention to the signature 'ripping' in signature.py
Made some guesses where things might go (e.g., tests, the iscython utility function), but they might be wrong

AmplabJenkins · 2017-11-08T02:29:04Z

Can one of the admins verify this patch?

pcmoritz · 2017-11-08T05:23:35Z

ok to test

pcmoritz · 2017-11-08T05:27:41Z

examples/cython/README.md

+- 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.


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

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

pcmoritz · 2017-11-08T05:33:12Z

python/ray/utils.py

@@ -12,6 +12,21 @@
 import ray.local_scheduler


+def iscython(obj):


Please rename this to is_cython, while Python often drops the _, in Ray we don't to make it more readable.

Fair enough. I'll change.

pcmoritz · 2017-11-08T05:37:10Z

test/cython_test.py

+import cython_examples as cyth
+
+
+def getRayResult(cython_func, *args):


get_ray_result

I am deeply ashamed.

pcmoritz · 2017-11-08T05:45:02Z

python/ray/signature.py

@@ -27,6 +29,39 @@
 """


+def get_signature_params(func):
+    """Get signature parameters
+    Probably the hackiest thing imaginable to support Cython built-ins. Create


Hm, if there is a better way to do this, consider doing it that way (I don't see one right now); otherwise maybe make the comment sound less terrible, given all things I've seen people do with (to) Python, this is actually not that hacky ;)

Hah! I'll admit the comment was more CYA, so if you guys are OK with it, then I'll reword the comment since I don't have a better answer. There's been some chatter in the Cython community (one example), but it seems that they're waiting on better Python support before doing anything about it.

pcmoritz

+1 There are a few nitpicks, otherwise this looks really great! Thanks for contributing this, the Cython functionality will be great (I'm glad you got it working and also glad about the tests). Also locations of the files looks good to me.

AmplabJenkins · 2017-11-08T05:49:31Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-11-08T05:49: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/2297/
Test FAILed.

pcmoritz · 2017-11-08T06:01:45Z

The Jenkins test failure is unrelated cc @richardliaw @ericl

danielsuo · 2017-11-08T13:30:52Z

@pcmoritz Quick question: I saw on some other issue you guys explained why Travis and Jenkins, but I just can't seem to find it. So: why Travis and Jenkins (besides Travis time-outs)?

AmplabJenkins · 2017-11-08T14:16:44Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-11-08T14:16:45Z

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

pcmoritz · 2017-11-08T16:58:34Z

@danielsuo We do some heavier weight distributed tests in Jenkins; it is preferable to do it in Jenkins because the machines it runs on are more powerful and the Travis builds are already quite long as it is. We could do everything in Jenkins but Travis has a very nice UI so we like it for the lighter weight tests.

richardliaw · 2017-11-08T18:58:19Z

examples/cython/cython_main.py

+
+@cli.command()
+def example1():
+    "Cython def function"


Is there a particular reason for 1 quote commenting (as opposed to three from above?)

Good catch! I also noticed that ray prefers double to single quotes where possible, so I changed all those as well.

richardliaw · 2017-11-08T19:00:10Z

python/ray/signature.py

+            raise TypeError('{0!r} is not a Python function we can process'
+                            .format(func))
+
+    return [(k, v) for k, v


nit: for readability will return list(funcsigs.signature(func).parameters.items()) work?

(ah I see this is vestige code from previously; but still would be nice to change to above)

Good suggestion. Done.

richardliaw

Few comments above :)

danielsuo · 2017-11-08T21:34:29Z

@pcmoritz got it, thanks for the repeated explanation.

Separately, related to #1191 and #1185, how should I prepare the branch for merging? It seems like a rebase to the main branch at the end of discussion is what typically happens.

AmplabJenkins · 2017-11-08T21:56:52Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-11-08T21:56:53Z

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

pcmoritz · 2017-11-08T22:02:18Z

According to the github UI there don't seem to be merge conflicts; however Travis doesn't seem to run which normally only happens if there are merge conflicts. We can wait a bit longer and then if Travis still hasn't run the last commit, you can do a rebase anyways and force push and see if that helps.

robertnishihara

Thanks for this patch, this is really nice work! I left a few more comments (most extremely minor).

If you don't mind me pushing to your branch, I'm happy to make the changes myself.

robertnishihara · 2017-11-09T05:13:11Z

.travis/install-cython-examples.sh

+   python setup.py install --user
+   popd
+
+elif [[ "$LINT" == "1" ]]; then


Let's not install anything in the linting builds

Ah yes, you're right.

robertnishihara · 2017-11-09T05:14:16Z

examples/cython/.gitignore

@@ -0,0 +1,6 @@
+build


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

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.

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.

robertnishihara · 2017-11-09T05:19:14Z

examples/cython/cython_examples/__init__.py

@@ -0,0 +1,29 @@
+from __future__ import absolute_import


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

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.

robertnishihara · 2017-11-09T05:30:58Z

examples/cython/README.md

@@ -0,0 +1,28 @@
+# Cython in Ray


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.

robertnishihara · 2017-11-09T05:31:46Z

examples/cython/README.md

+- 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.


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

robertnishihara · 2017-11-09T05:33:32Z

examples/cython/cython_examples/masked_log.pyx

+
+def masked_log(x):
+    """x is a 1D numpy array"""
+    """returns -Inf for x <=0 and log(x) otherwise"""


Probably should only have one doc string.

Lalala not my fault all upstream. Updated to latest upstream.

robertnishihara · 2017-11-09T06:45:56Z

@danielsuo there's no need to rebase on the master. We're just confused about why the Travis tests weren't automatically running. But my guess is if we push another commit on top of this then that will trigger the Travis tests.

danielsuo · 2017-11-09T15:18:26Z

Weird, the new commit isn't triggering Travis either. Typically this would happen if the PR isn't mergeable, but just tested and there's nothing conflicting with master. Nothing looks out of the ordinary with Travis either (besides, of course, not building on the last two commits).

My Travis was triggered (still has trouble with timeouts and not receiving stdout in 10mins for MacOS).

danielsuo · 2017-11-09T15:26:07Z

@robertnishihara appreciate the offer to help, but it seems that you have more than enough on your plate!

AmplabJenkins · 2017-11-09T15:37:09Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-11-09T15:37: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/2326/
Test PASSed.

AmplabJenkins · 2017-11-09T15:57:15Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-11-09T15:57:16Z

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

AmplabJenkins · 2017-11-09T21:17:55Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-11-09T21:17:55Z

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

robertnishihara · 2017-11-09T21:42:57Z

I'm not really sure what's going on with Travis, so I duplicated the PR and am running tests in #1202.

pcmoritz · 2017-11-10T01:49:02Z

Tests passed on #1202

danielsuo · 2017-11-10T02:28:08Z

Very easy PR process, much appreciated!

Add basic functionality for Cython functions and actors

10f0cc4

pcmoritz reviewed Nov 8, 2017

View reviewed changes

Fix up per @pcmoritz comments

833f5ac

richardliaw reviewed Nov 8, 2017

View reviewed changes

Fixes per @richardliaw comments

c195227

robertnishihara reviewed Nov 9, 2017

View reviewed changes

Fixes per @robertnishihara comments

47653e1

Forgot double quotes when updating masked_log

62dce55

robertnishihara mentioned this pull request Nov 9, 2017

DO NOT MERGE: Duplicate of #1193 #1202

Closed

robertnishihara approved these changes Nov 9, 2017

View reviewed changes

Remove import typing for Python 2 compatibility

6388593

pcmoritz merged commit 4f0da6f into ray-project:master Nov 10, 2017

robertnishihara mentioned this pull request Nov 10, 2017

Ray does not support cython? #944

Closed

danielsuo deleted the cython branch November 16, 2017 13:42

robertnishihara mentioned this pull request May 17, 2018

Support calling positional arguments by keyword (fix #998) #2081

Merged

		@@ -12,6 +12,21 @@
		import ray.local_scheduler


		def iscython(obj):

		import cython_examples as cyth


		def getRayResult(cython_func, *args):

Add basic functionality for Cython functions and actors #1193

Add basic functionality for Cython functions and actors #1193

Conversation

danielsuo commented Nov 8, 2017 • edited Loading

AmplabJenkins commented Nov 8, 2017

pcmoritz commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcmoritz Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcmoritz left a comment • edited Loading

Choose a reason for hiding this comment

AmplabJenkins commented Nov 8, 2017

AmplabJenkins commented Nov 8, 2017

pcmoritz commented Nov 8, 2017

danielsuo commented Nov 8, 2017

AmplabJenkins commented Nov 8, 2017

AmplabJenkins commented Nov 8, 2017

pcmoritz commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardliaw Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardliaw left a comment • edited Loading

Choose a reason for hiding this comment

danielsuo commented Nov 8, 2017

AmplabJenkins commented Nov 8, 2017

AmplabJenkins commented Nov 8, 2017

pcmoritz commented Nov 8, 2017

robertnishihara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielsuo Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertnishihara commented Nov 9, 2017

danielsuo commented Nov 9, 2017 • edited Loading

danielsuo commented Nov 9, 2017

AmplabJenkins commented Nov 9, 2017

AmplabJenkins commented Nov 9, 2017

AmplabJenkins commented Nov 9, 2017

AmplabJenkins commented Nov 9, 2017

AmplabJenkins commented Nov 9, 2017

AmplabJenkins commented Nov 9, 2017

robertnishihara commented Nov 9, 2017

pcmoritz commented Nov 10, 2017

danielsuo commented Nov 10, 2017

danielsuo commented Nov 8, 2017 •

edited

Loading

pcmoritz Nov 8, 2017 •

edited

Loading

pcmoritz left a comment •

edited

Loading

richardliaw Nov 8, 2017 •

edited

Loading

richardliaw left a comment •

edited

Loading

danielsuo Nov 9, 2017 •

edited

Loading

danielsuo commented Nov 9, 2017 •

edited

Loading