-
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
Fixes #14: Refactoring ray portable runner #18
Fixes #14: Refactoring ray portable runner #18
Conversation
r: @iasoon ? : ) |
I realize that CI is broken. Should we fix it as part of this PR? or as a follow-up PR? |
21f68b3
to
a5dff00
Compare
a5dff00
to
c6a36ca
Compare
I made all the portability runner tests run : ) |
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!
I can't comment on all code equally well, but I feel this version is generally cleaner indeed 👍
return self.transform_to_buffer_coder, self.data_output, self.stage_timers | ||
|
||
def setup(self): |
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 there a reason why this method is only called after initialization of the ContextManager
?
(transform_id, id) | ||
] = timer_family_spec.timer_family_coder_id | ||
|
||
def __reduce__(self): |
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.
Nit / general remark: It's a bit unfortunate that we keep running into the serialization issue, and sometimes solve it by using a custom __reduce__
, sometimes by registering a custom serializer (ray.util.register_serializer
), and sometimes manually (SerializeToString
/ FromString
).
It would be good if we could enable serialization for all the protobuf components in one central place - I'm not sure how that could be done though, as ray.util.register_serializer
would have to be called on every ray worker that transmits protobuf objects. Maybe something to discuss?
TODO: Rename immutable context managers to |
Sounds good to me :) |
Making the code more readable and more efficient.
Also adding support for SDF-initiated checkpointing