Move function/actor exporting & loading code to function_manager.py #3003
Conversation
|
Test FAILed. |
|
@suquark can you take a look at this? |
|
Thanks @guoyuhong! Can you say if there are any behavior changes or if it is only code reorganization? |
|
@robertnishihara I think it is similiar to #2602 where I think this commit along with previous trials indicate that refactoring Ray's core components will definitely changes hundreds lines of codes because current code is highly coupled. This will make code review slow. However, if we just postpone the refactoring, our situation will be harder in the future. That's our problem. |
|
So my suggestion is that we should be more decisive in refactoring. We should first discuss when to start refactoring (because we are still building a new release) and define a time window in which to complete the refactoring tasks. |
|
Thanks @suquark, this PR feels fairly self-contained to me, so while I agree that major refactoring to worker.py should probably be put off until 0.6 is released, incremental cleanups that can be done quickly are probably a good idea. |
|
@robertnishihara @suquark There is no behavior change in this PR. However, we may further leverage this PR to implement the function descriptor as @raulchen talked about the function descriptor change in the meeting. The next step, we will have a PR that change the backend code to remove the function id and use function descriptor in python. |
|
One more question, when will Version0.6 be released? |
|
@guoyuhong we're tracking tasks for 0.6 in https://github.com/ray-project/ray/projects/5. The To do column is the highest priority stuff and the Optional backlog column is optional. Probably somewhere between the middle of October and the end of October. |
|
@robertnishihara Thanks for the clarification. Will this PR be accepted right now or only after Release0.6? |
|
I think it should be merged before. I’ll try to review it later today or
tomorrow. I’ll discuss with @suquark.
…On Tue, Oct 2, 2018 at 8:10 PM Yuhong Guo ***@***.***> wrote:
@robertnishihara <https://github.com/robertnishihara> Thanks for the
clarification. Will this PR be accepted right now or only after Release0.6?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3003 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAPOrd2BlivpTtH3mw0kDdmsIxUpcQaKks5uhCqfgaJpZM4XCUUP>
.
|
There was a problem hiding this comment.
Left very minor comments. I guess the main question is whether this should be a "FunctionManager" which managers remote functions and actors, or whether we should generalize it to handle all importing/exporting of Python functions (e.g., remote functions, actors, and "functions to run" (custom serializers)).
The more general approach is what @suquark did in a different PR, and I think that's probably the direction we want to go in (though probably in a follow up PR).
python/ray/actor.py
Outdated
| """Save a checkpoint on the actor and log any errors. | ||
| Args: | ||
| Args: |
There was a problem hiding this comment.
newline before Args:
python/ray/actor.py
Outdated
| """Restore an actor from a checkpoint and log any errors. | ||
| Args: | ||
| Args: |
There was a problem hiding this comment.
newline before Args:
| namedtuple, | ||
| defaultdict, | ||
| ) | ||
| import ray |
There was a problem hiding this comment.
newline before ray imports
python/ray/function_manager.py
Outdated
|
|
||
| class FunctionManager(object): | ||
| """ | ||
| A class used to export/load remote functions. |
There was a problem hiding this comment.
Should be
"""A class used to export/load remote functions.
python/ray/function_manager.py
Outdated
| A class used to export/load remote functions. | ||
| Attributes: | ||
There was a problem hiding this comment.
Please add the documentation
python/ray/function_manager.py
Outdated
|
|
||
| def _do_export(self, remote_function): | ||
| """ | ||
| Pickle a remote function and export it to redis. |
There was a problem hiding this comment.
Move to previous line
python/ray/function_manager.py
Outdated
| def __init__(self, worker): | ||
| self._worker = worker | ||
| self._functions_to_export = [] | ||
| self._actor_to_export = [] |
There was a problem hiding this comment.
probably should be _actors_to_export.
python/ray/function_manager.py
Outdated
|
|
||
| def get_execution_info(self, driver_id, function_id): | ||
| """ | ||
| Get the FunctionExecutionInfo of a remote function. |
There was a problem hiding this comment.
Move to previous line
python/ray/function_manager.py
Outdated
| Args: | ||
| driver_id: id of the driver that the function belongs to. | ||
| function_id: id of the function to get. |
There was a problem hiding this comment.
id -> ID, in both lines above
python/ray/function_manager.py
Outdated
| time.sleep(0.001) | ||
|
|
||
| @classmethod | ||
| def function_predictor(cls, x): |
There was a problem hiding this comment.
Maybe is_function_or_method?
|
@robertnishihara Thanks for the comments and communicating with @suquark . Yes, @suquark 's change may be a better choice for long term planning. Will his change be merged only after Release 0.6? Current PR is important to us because we want to enable calling Python code from Java as soon as possible due to the schedule. |
python/ray/function_manager.py
Outdated
| time.sleep(0.001) | ||
|
|
||
| @classmethod | ||
| def is_function_or_method(cls, x): |
There was a problem hiding this comment.
Could we put is_function_or_method & is_classmethod into ray.utils? They are more like general type inspecting tools.
python/ray/function_manager.py
Outdated
|
|
||
| # Create a temporary actor with some temporary methods so that if | ||
| # the actor fails to be unpickled, the temporary actor can be used | ||
| # (just to produceerror messages and to prevent the driver from |
|
This PR looks good for me. This could also be helpful for further worker refactoring. |
|
Test FAILed. |
|
Thanks @suquark! I went ahead and made this changes. Will merge assuming tests pass. |
|
Test PASSed. |
1. Remove a comment that should have been removed in ray-project#3003 2. Remove `redis_protected_mode` that is never used in `ray.init()` 3. Fix `object_id_seed` that is forgotten to be passed into `ray._init()` 4. Remove several redundant brackets.
This commit fix some small defects. 1. Remove a comment that should have been removed in #3003 2. Remove `redis_protected_mode` that is never used in `ray.init()` 3. Fix `object_id_seed` that is forgotten to be passed into `ray._init()` 4. Remove several redundant brackets.
|
@robertnishihara @suquark Thanks! |
5 participants
What do these changes do?
Move function/actor exporting & loading code to function_manager.py to prepare the code change for function descriptor for python.
Related issue number
#2576