-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add some comments for distribute_transpiler #9735
Conversation
@@ -192,22 +194,24 @@ def transpile(self, | |||
self.trainer_id = trainer_id | |||
pserver_endpoints = pservers.split(",") | |||
|
|||
# step1 | |||
# step1: For large parameters and gradients, split them into smaller |
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.
Line 154 has comments already.
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 think the L154 is a good place for high level descriptions. inlining detail comments within the codes is better for readers. They can co-exists.
@@ -812,6 +824,9 @@ def _get_lr_ops(self): | |||
find_ops.append(op) | |||
# make a union find struct by the ops in default_main_program | |||
ufind = UnionFind(block.ops) | |||
|
|||
# TODO(panyx0718): If lr_ops connects with other training |
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.
We'll use SSA to replace the current ufind structure. For current deeplearning programs, it works fine.
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.
Removed this comment
@@ -278,6 +283,8 @@ def get_pserver_program(self, endpoint): | |||
orig_var_name = v.name[:suff_idx] | |||
else: | |||
orig_var_name = v.name | |||
#TODO(panyx0718): Should this be put in the else block below? It's |
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.
Can put directly under else
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.
Done
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 very much, LGTM!
No description provided.