Rename variables and add comments #27286

pietern · 2019-10-03T14:00:29Z

Stack from ghstack:

Run clang-format for torch/distributed/rpc #27531 Run clang-format for torch/distributed/rpc
Rename PythonUDF{Call,Resp} #27530 Rename PythonUDF{Call,Resp}
Scope pybind11 functions to torch.distributed.{autograd,rpc} #27529 Scope pybind11 functions to torch.distributed.{autograd,rpc}
Move RPC API to torch.distributed.rpc #27290 Move RPC API to torch.distributed.rpc
Move internal functions to torch.distributed.rpc #27289 Move internal functions to torch.distributed.rpc
Move RPC backend registry to torch.distributed.rpc #27288 Move RPC backend registry to torch.distributed.rpc
Remove torch.distributed.rpc function #27287 Remove torch.distributed.rpc function
Rename variables and add comments #27286 Rename variables and add comments
Remove shebang from non-executable files in torch.distributed #27285 Remove shebang from non-executable files in torch.distributed
Fix pybind11 warnings in python_rpc_handler.cpp #27284 Fix pybind11 warnings in python_rpc_handler.cpp

The name runUDFFunction stutters because the F in UDF also stands
for function. Renamed these variables to be identical to their Python
equivalents. Renamed those to share a prefix and drop internal,
because internal functions can use an underscore prefix.

Differential Revision: D17808208

The name `runUDFFunction` stutters because the F in UDF also stands for function. Renamed these variables to be identical to their Python equivalents. Renamed those to share a prefix and drop `internal`, because internal functions can use an underscore prefix.

xush6528 · 2019-10-03T18:31:27Z

torch/csrc/distributed/rpc/python_rpc_handler.cpp

      py::module::import("torch.distributed.internal_rpc_utils");
-  runUDFFunction_ = getFunction(module, "run_python_udf_internal");
-  loadResultFunction_ = getFunction(module, "load_python_udf_result_internal");
+  pythonUDFRun_ = getFunction(module, "_python_udf_run");


Do you think "Python" + "UDF" is a good name here?

It's just any Python function.

I learned the word UDF in SQL, SparkSQL, where UDF is somoething beyond the built-in capability of the domain-specific language.

Good point. I'll use @mrshenli's suggestion below.

xush6528 · 2019-10-03T18:32:29Z

torch/distributed/internal_rpc_utils.py

+    Unpickles a Python function and its arguments, runs the function,
+    and pickles and returns the function's return value.
    """
    python_udf = _internal_rpc_pickler.deserialize(pickled_python_udf, tensors)


The name "_python_udf_run" doesn't indicate it's actually taking a serialized python function with args, deserialize it, run it, and serialize it.

Should we call it something else?

_python_udf_remote_run?

Let's try to avoid using remote here, as dist.remote is another API with a different scope.

In the message level, we have PYTHON_CALL and PYTHON_RET. Shall we use a similar name here? For example:

_python_udf_run -> _run_serialized_python_call

load_python_udf_result_internal -> _deserialize_python_ret

Good points. I stuck with the original naming because I didn't know about the intent here. As it can be any Python function, I think @mrshenli's suggestions are good.

zhaojuanmao

thanks for improving it, lgtm

zhaojuanmao · 2019-10-03T22:09:50Z

torch/distributed/internal_rpc_utils.py

+    Unpickles a Python function and its arguments, runs the function,
+    and pickles and returns the function's return value.
    """
    python_udf = _internal_rpc_pickler.deserialize(pickled_python_udf, tensors)


_python_udf_remote_run?

The name `runUDFFunction` stutters because the F in UDF also stands for function. Renamed these variables to be identical to their Python equivalents. Renamed those to share a prefix and drop `internal`, because internal functions can use an underscore prefix.

Per discussion in #27286, the `UDF` part is superfluous. This makes the naming consistent with the `MessageType` enum.

pietern · 2019-10-07T22:25:51Z

@mrshenli @zhaojuanmao @xush6528 Renamed to remove UDF and added py prefix to indicate the C++ code is calling into Python code (which is good to know).

zhaojuanmao

lgtm

Per discussion in #27286, the `UDF` part is superfluous. This makes the naming consistent with the `MessageType` enum.

xush6528

Thanks. Looks better!

Per discussion in #27286, the `UDF` part is superfluous. This makes the naming consistent with the `MessageType` enum.

facebook-github-bot · 2019-10-08T20:34:28Z

@pietern merged this pull request in 48a571b.

facebook-github-bot · 2019-10-08T20:34:38Z

@pietern merged this pull request in 48a571b.

Per discussion in #27286, the `UDF` part is superfluous. This makes the naming consistent with the `MessageType` enum. Differential Revision: [D17808211](https://our.internmc.facebook.com/intern/diff/D17808211)

Summary: Pull Request resolved: #27530 Per discussion in #27286, the `UDF` part is superfluous. This makes the naming consistent with the `MessageType` enum. Test Plan: Imported from OSS Differential Revision: D17808211 Pulled By: pietern fbshipit-source-id: 0ff925de26d027951ce285750ad276ed17fee4c6

Summary: Pull Request resolved: pytorch#27286 The name `runUDFFunction` stutters because the F in UDF also stands for function. Renamed these variables to be identical to their Python equivalents. Renamed those to share a prefix and drop `internal`, because internal functions can use an underscore prefix. Test Plan: Imported from OSS Differential Revision: D17808208 Pulled By: pietern fbshipit-source-id: 7619f07fc8215203dfb1da1eb281845edcd2bb99

pietern requested review from apaszke and mrshenli as code owners October 3, 2019 14:00

pytorchbot added oncall: distributed Add this issue/PR to distributed oncall triage queue module: pybind Related to our Python bindings / interactions with other Python libraries labels Oct 3, 2019

xush6528 reviewed Oct 3, 2019

View reviewed changes

mrshenli requested a review from zhaojuanmao October 3, 2019 20:20

zhaojuanmao approved these changes Oct 3, 2019

View reviewed changes

mrshenli approved these changes Oct 4, 2019

View reviewed changes

xush6528 approved these changes Oct 4, 2019

View reviewed changes

pietern added a commit that referenced this pull request Oct 7, 2019

Rename PythonUDF{Call,Resp}

c24a419

Per discussion in #27286, the `UDF` part is superfluous. This makes the naming consistent with the `MessageType` enum.

This was referenced Oct 7, 2019

Rename PythonUDF{Call,Resp} #27484

Closed

Scope pybind11 functions to torch.distributed.{autograd,rpc} #27483

Closed

Run clang-format for torch/distributed/rpc #27485

Closed

pietern added a commit that referenced this pull request Oct 7, 2019

Update on "Rename PythonUDF{Call,Resp}"

a4046de

Per discussion in #27286, the `UDF` part is superfluous. This makes the naming consistent with the `MessageType` enum.

pietern requested review from mrshenli, xush6528 and zhaojuanmao October 7, 2019 22:24

zhaojuanmao approved these changes Oct 7, 2019

View reviewed changes

pietern added a commit that referenced this pull request Oct 7, 2019

Update base for Update on "Rename PythonUDF{Call,Resp}"

e02b323

Per discussion in #27286, the `UDF` part is superfluous. This makes the naming consistent with the `MessageType` enum.

pietern added a commit that referenced this pull request Oct 7, 2019

Update on "Rename PythonUDF{Call,Resp}"

1973758

Per discussion in #27286, the `UDF` part is superfluous. This makes the naming consistent with the `MessageType` enum.

xush6528 approved these changes Oct 7, 2019

View reviewed changes

pietern added a commit that referenced this pull request Oct 8, 2019

Rename PythonUDF{Call,Resp}

eab84b9

Per discussion in #27286, the `UDF` part is superfluous. This makes the naming consistent with the `MessageType` enum.

This was referenced Oct 8, 2019

Rename PythonUDF{Call,Resp} #27528

Closed

Rename PythonUDF{Call,Resp} #27530

Closed

pietern added a commit that referenced this pull request Oct 8, 2019

Rename PythonUDF{Call,Resp}

332a883

Per discussion in #27286, the `UDF` part is superfluous. This makes the naming consistent with the `MessageType` enum.

This was referenced Oct 8, 2019

Scope pybind11 functions to torch.distributed.{autograd,rpc} #27529

Closed

Run clang-format for torch/distributed/rpc #27531

Closed

pietern added a commit that referenced this pull request Oct 8, 2019

Update on "Rename PythonUDF{Call,Resp}"

6e3e162

Per discussion in #27286, the `UDF` part is superfluous. This makes the naming consistent with the `MessageType` enum.

facebook-github-bot closed this in 48a571b Oct 8, 2019

facebook-github-bot added the merged label Oct 8, 2019

facebook-github-bot deleted the gh/pietern/41/head branch October 28, 2019 22:18

mruberry added the Merged label Oct 28, 2020

Rename variables and add comments #27286

Rename variables and add comments #27286

Uh oh!

Conversation

pietern commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xush6528 Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

pietern Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

xush6528 Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

zhaojuanmao Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

mrshenli Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

pietern Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

zhaojuanmao left a comment

Choose a reason for hiding this comment

Uh oh!

zhaojuanmao Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

pietern commented Oct 7, 2019

Uh oh!

zhaojuanmao left a comment

Choose a reason for hiding this comment

Uh oh!

xush6528 left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 8, 2019

Uh oh!

facebook-github-bot commented Oct 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

pietern commented Oct 3, 2019 •

edited

Loading