add reusable for win remote #3500
Conversation
| @@ -31,6 +32,8 @@ import { SharedStorageService, SharedStorageConfig } from './sharedStorage'; | |||
| import { NFSSharedStorageService } from './shared_storages/nfsStorageService' | |||
| import { AzureBlobSharedStorageService } from './shared_storages/azureblobStorageService' | |||
| import { TrialDetail } from './trial'; | |||
| import { LinuxCommands } from "../remote_machine/extends/linuxCommands"; | |||
There was a problem hiding this comment.
trialDispatcher.ts is a common service, should not import common commands from remote_machine folder.
| environment.command = `mkdir -p envs/${envId} && cd envs/${envId} && ${environment.command}`; | ||
|
|
||
| environment.command = new LinuxCommands().reusableStartCommand(envId, this.isDeveloping); | ||
| environment.commandforwin = new WindowsCommands().reusableStartCommand(envId, this.isDeveloping); |
There was a problem hiding this comment.
This commandforwin seems only for remote windows mode in this PR, don't put it here, this trialDispatcher is for common commands. Suggest to refer localEnvironmentService to see how it process local windows commands, or refactor logic to extract windows command and put it here.
|
|
||
| environment.command = `mkdir -p envs/${envId} && cd envs/${envId} && ${environment.command}`; | ||
|
|
||
| environment.command = `python3 -m nni.tools.trial_tool.trial_runner`; |
There was a problem hiding this comment.
This change will break other training service.
| } else { | ||
| const prepare = `mkdir -p envs/${environment.envId} && cd envs/${environment.envId}`; | ||
| const startrun = `sh ../install_nni.sh && python3 -m nni.tools.trial_tool.trial_runner`; | ||
| const developingScript = "[ -d \"nni_trial_tool\" ] && echo \"nni_trial_tool exists already\" || (mkdir ./nni_trial_tool && tar -xof ../nni_trial_tool.tar.gz -C ./nni_trial_tool) && pip3 install websockets"; |
There was a problem hiding this comment.
Seems these linux commands already set in trialDispatcher.ts and could use original commands directly, only need to add windows comands in remoteEnvironmentService.ts?
| @@ -225,8 +225,10 @@ class TrialDispatcher implements TrainingService { | |||
| const codeFileName = await storageService.copyDirectory(codeDir, envDir, true); | |||
| storageService.rename(codeFileName, "nni-code.tar.gz"); | |||
|
|
|||
| const installFileName = storageService.joinPath(envDir, 'install_nni.sh'); | |||
| const installFileName = storageService.joinPath(envDir, `install_nni.sh`); | |||
| const installFileNameForWin = storageService.joinPath(envDir, `install_nni.cmd`); | |||
There was a problem hiding this comment.
For other places in nni using .ps1 under windows, better to unify .ps1 in this place.
|
|
||
| if (executor.getIsWindows()) { | ||
| const prepare = `envs\\install_nni.cmd && mkdir envs\\${environment.envId} && cd envs\\${environment.envId}`; | ||
| const startrun = `cd .. && install_nni.cmd && cd ${environment.envId} && python -m nni.tools.trial_tool.trial_runner`; |
There was a problem hiding this comment.
seems install twice? envs\\install_nni.cmd and install_nni.cmd
and multi cd can be simplified.
| @@ -110,6 +109,15 @@ class ShellExecutor { | |||
| this.sshClient.end(); | |||
| } | |||
|
|
|||
| public async getIsWindows(): Promise<boolean>{ | |||
| const result = await this.execute("ver") | |||
There was a problem hiding this comment.
We have ShellExecutor.isWindows to check if is windows, if necessary, change isWindows to public.
| script = logLevel == "debug" ? `${prepare} && ${developingScript} && ${startrun}` : `${prepare} && ${startrun}`; | ||
| } | ||
|
|
||
| script = `cd ${environment.runnerWorkingFolder} && \ |
There was a problem hiding this comment.
this script value will override line 221.
There was a problem hiding this comment.
It only works for windows "isdebug" mode or not, Default(Linux) is not affected.
acured
force-pushed
the
AddReusableForWinRemote
branch
from
281c043 to
e32bda9
Compare
ts/nni_manager/training_service/reusable/environments/remoteEnvironmentService.ts
Show resolved
Hide resolved
No description provided.