New design for launch/run #40086
New design for launch/run #40086
Conversation
|
Thanks for your contribution! |
kuizhiqing
force-pushed
the
run
branch
2 times, most recently
from
076107d
to
dbd20ff
Compare
| "--rank", type=int, default=-1, help="the peer rank") | ||
|
|
||
| base_group.add_argument( | ||
| "--log", type=str, default="INFO", help="log level. Default INFO") |
There was a problem hiding this comment.
I think log_level or verbose is more meaningful.
| "--np", | ||
| type=str, | ||
| default="1", | ||
| help="the number of peers, i.e. pod/node number") |
There was a problem hiding this comment.
In MPI, np means the total number of processes instead of number of nodes.
There was a problem hiding this comment.
I also think it is better to use another name.
There was a problem hiding this comment.
ok, replace with nnodes later
| "--np", | ||
| type=str, | ||
| default="1", | ||
| help="the number of peers, i.e. pod/node number") |
There was a problem hiding this comment.
I also think it is better to use another name.
| sys.exit(sigint) | ||
|
|
||
|
|
||
| class Controller(ControllerBase): |
There was a problem hiding this comment.
Why do we need both Controller and ControllerBase as the base class?
There was a problem hiding this comment.
ControllerBase refer to really basic workflow, Controller holds more API. Nothing but separation.
| def replicas(self, replicas): | ||
| self._replicas = replicas | ||
|
|
||
| def set_replicas(self, np: str): |
There was a problem hiding this comment.
Does the replicas also mean the number of nodes？
There was a problem hiding this comment.
A little diff, replicas refers to logical nodes number, since one nodes may deploy two or more replicas.
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| cli = PKVClient("http://localhost:8090") |
There was a problem hiding this comment.
Never Mind, just for local testing.
| return 0 | ||
|
|
||
| def deploy(self): | ||
| for i in self._init_containers: |
There was a problem hiding this comment.
If my understanding is right, the init_containers and conatiners are called by this launch module and users don't need to do the relaunch in their own scripts.
|
|
||
| assert self.client.wait_server_ready(timeout=600), 'server is not ready' | ||
|
|
||
| # 'aaaaaa' make suer main pod (master server) as rank 0 |
| help="seconds to wait before elastic perform training") | ||
| return parser.parse_args() | ||
|
|
||
| def _valide_env(self, key): |
| return parser.parse_args() | ||
|
|
||
| def _valide_env(self, key): | ||
| if key in ['POD_IP']: |
There was a problem hiding this comment.
POD_IP（历史遗留）是k8s里的概念，用在这里其实并不是特别合适，是否要用一个新的名称，同时保留兼容POD_IP，后续逐步废弃POD_IP?
There was a problem hiding this comment.
是的，不过目前 role_maker 里面用了，目前需要兼容，后续可以考虑替换
| def set_env_in_args(self): | ||
| env_args = { | ||
| 'POD_IP': 'host', | ||
| 'PADDLE_MASTER': 'master', |
There was a problem hiding this comment.
相同名称的环境变名称字符串分散在多个py文件中，是否需要将相关环境变量统一定义？
There was a problem hiding this comment.
目前所有除device相关的环境变量都统一在这里了，且在这里定义 args 和 env 的对应关系
There was a problem hiding this comment.
可以在check一下，"PADDLE_MASTER"�在其他地方也出现了�
There was a problem hiding this comment.
出现的另一个 PADDLE_MASTER 并不是同一个，后者取决于 fleet api 里面的定义。launch/run 模块用到的环境变量在 context。
| # python -m paddle.distributed.run train.py | ||
|
|
||
| use specified devices | ||
| # python -m paddle.distributed.run --devices=0,1,2,3 train.py |
There was a problem hiding this comment.
what's the difference between "paddle.distributed.launch fleetrun" and "paddle.distributed.run"? Why not reuse launch
There was a problem hiding this comment.
As some args may not compatible with launch, we use run in this version to avoid breaking change.
In the new design, run module is a self contained directory, which can replace launch by renaming in the stable version.
PR types
New features
PR changes
Others
Describe
This pr add new launch method for paddle, including elastic support.
More details in python/paddle/distributed/run/init.py