-
Notifications
You must be signed in to change notification settings - Fork 12
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
Initial Ray runner prototype based on FnApiRunner in Beam #10
Conversation
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.
LGTM! I think we can merge this and iterate on top of it.
@@ -84,6 +85,18 @@ def contains_labels(mi, labels): | |||
return contains_labels(mi, labels) and mi.urn == urn | |||
|
|||
|
|||
class RayFnRunnerTest(fn_runner_test.FnApiRunnerTest): |
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.
Is this used?
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.
Awesome work - thanks Pablo!
It may be a good idea to start capturing some of the TODOs and comments here as issues for others to start picking up.
I was able to get the Ray Runner tests passing with one change related to WatermarkManager
initialization. Can you verify whether or not this is expected? I just had a couple other minor comments, but otherwise think this looks ready to merge!
"ray[data]", "apache_beam" | ||
], | ||
extras_require={ | ||
'test': TEST_REQUIREMENTS, |
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 for separating out the test requirements as extras! Can we also add them to requirements_dev.txt
with a comment noting that they are only required for test execution? Ideally, I think our install_requires
dependencies should also be mirrored in requirements_dev.txt
for ease of developer dependency management.
LGTM. Pablo, can we have a demo on this later sometime? |
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've committed the minor change required to make supported tests pass - merging. Thanks, Pablo!
thanks Patrick, everyone. I'll try to create new work items for this. @wilsonwang371 I'll be happy to do a demo on this. |
This prototype is hacked together as quickly as possible. I can't speak for its efficiency, and its code cleanliness.
Next steps:
ray_runner_test.py
Others...