Rename rollout_engines and rank to reflect semantics#934
Rename rollout_engines and rank to reflect semantics#934fzyzcjy wants to merge 2 commits intorollout_ft/19from
Conversation
There was a problem hiding this comment.
Code Review
This pull request renames the rollout_engines variable to new_engines in server_group.py and adds a maintenance note in addr_allocator.py. The review feedback correctly identifies that renaming the rank variable to index in the engine initialization loop reduces semantic clarity, as the value represents a global rank in a distributed computing context.
| **addr_and_ports[index], | ||
| router_ip=self.router_ip, | ||
| router_port=self.router_port, | ||
| ) | ||
| for rank, engine in rollout_engines | ||
| for index, engine in new_engines |
There was a problem hiding this comment.
Renaming rank to index here is less descriptive and potentially misleading. In distributed computing contexts, "rank" is the standard term for a process identifier within a group, whereas "index" typically refers to a position in a sequence (like a 0-based list index). Since the value being unpacked from new_engines is the global_rank (as seen in line 120), it is better to maintain the name rank or global_rank to reflect its actual semantics and maintain consistency with the rest of the codebase.
| **addr_and_ports[index], | |
| router_ip=self.router_ip, | |
| router_port=self.router_port, | |
| ) | |
| for rank, engine in rollout_engines | |
| for index, engine in new_engines | |
| **addr_and_ports[rank], | |
| router_ip=self.router_ip, | |
| router_port=self.router_port, | |
| ) | |
| for rank, engine in new_engines |
No description provided.