-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Cross-language invocation Part 1: Java calling Python functions and actors #4166
Conversation
Test FAILed. |
If we got the eternal bug https://issues.apache.org/jira/browse/ARROW-1692 resolved, we could pass data natively between Java arrow and python arrow :D |
public static RayPyActor createPyActor(String moduleName, String className, Object obj0, Object obj1, Object obj2, Object obj3, Object obj4, Object obj5, ActorCreationOptions options) { | ||
Object[] args = new Object[]{obj0, obj1, obj2, obj3, obj4, obj5}; | ||
return Ray.internal().createPyActor(moduleName, className, args, options); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these args are all untyped, why not use Java varargs? The only caveat is that you'll have to move the creation options a bit earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, varargs would look nicer. But I wanted to make the order aligned with the regular createActor
.
return className; | ||
} | ||
|
||
public RayPyActorImpl fork(boolean random) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the random arg for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, that's an internal hack. I'll remove it.
int language; | ||
int functionDescriptorOffset; | ||
|
||
if (task.functionDescriptor != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the nullability of functionDescriptor / pyFuncDescriptor confusing. Consider having one type for both, and methods task.functionDescriptor.getLanguage()
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good.
public final FunctionDescriptor functionDescriptor; | ||
|
||
// Descriptor of the target Python function. This field is only valid for Python tasks. | ||
public final PyFunctionDescriptor pyFunctionDescriptor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider merging these two fields to avoid nulls.
actorImpl.setTaskCursor(spec.returnIds[1]); | ||
actorImpl.clearNewActorHandles(); | ||
} | ||
rayletClient.submitTask(spec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
return actor; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many newlines?
java/pom.xml
Outdated
<groupId>com.google.code.gson</groupId> | ||
<artifactId>gson</artifactId> | ||
<version>2.8.5</version> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's gson for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually needed for the next PR (serializing handle with json and pass it to Python), forgot to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments.
Can one of the admins verify this patch? |
1 similar comment
Can one of the admins verify this patch? |
@raulchen any follow-up here? Looks like there are some merge conflicts now. |
@ericl I rebased the PR and addressed your comments. |
Test PASSed. |
Test PASSed. |
Test PASSed. |
Test PASSed. |
CI failure is related, but I cannot reproduce it on my machine. Will check tomorrow. |
/** | ||
* Represents metadata of a Python function. | ||
*/ | ||
public class PyFunctionDescriptor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of a little more type safety, how about having both this and FunctionDescriptor extend the same interface or base class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, will change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good -- just one comment on having a common base class for a bit more type safety.
It would also be nice to add an example of Python->Java calls, if there is a use case for that.
Test FAILed. |
This reverts commit 1dcec80.
@ericl Python calling Java will be implemented in next PR. Although it's already doable with low-level api, I'd like to create a higher-level API for convenience in next PR. I haven't thought thoroughly about the API, it may be something like the following. Suggestions are welcome!
|
Sounds good. I think the experimental syntax looks fine for an initial release; you could also try to wrap the actors to provide a bit more of a pythonic API. |
Test FAILed. |
Test FAILed. |
Test PASSed. |
What do these changes do?
Implements Java calling Python functions and actors.
Usage:
Note, currently arguments can only be raw binaries. And when putting a binary in object store, we use special metadata to indicate that the object is raw binary and doesn't need serialization/deserialization.
Not implemented in this PR:
Related issue number
#2576