Skip to content
This repository has been archived by the owner on Sep 18, 2024. It is now read-only.

Refactor code storage logic for trial #2403

Merged
merged 62 commits into from
May 19, 2020
Merged

Refactor code storage logic for trial #2403

merged 62 commits into from
May 19, 2020

Conversation

SparkSnail
Copy link
Contributor

@SparkSnail SparkSnail commented May 5, 2020 •
edited
Loading

  1. in local platform, copy code files to trial's workding dir.
  2. in other platform, copy code files to remote machine firstly, and then copy data in remote machine to trial's working folder, not submitted from local every time.

SparkSnail and others added 30 commits August 6, 2019 18:58
@SparkSnail
Merge pull request #200 from microsoft/master
704b50e 
merge master
@SparkSnail
Merge pull request #204 from microsoft/master
5b0034e 
merge master
@SparkSnail
Merge pull request #205 from microsoft/master
8fe2588 
Update evolution doc (microsoft#1493)
@SparkSnail
Merge pull request #206 from microsoft/master
9fae194 
merge master
@SparkSnail
Merge pull request #207 from microsoft/master
c785655 
merge master
@SparkSnail
Merge pull request #208 from microsoft/master
2f5272c 
merge master
@SparkSnail
Merge pull request #209 from microsoft/master
1892bc2 
augment pylintrc (microsoft#1643)
@SparkSnail
Merge pull request #210 from microsoft/master
7c1ab11 
fix console.log (microsoft#1636)
@SparkSnail
Merge pull request #211 from microsoft/master
8c203f3 
merge master
@SparkSnail
check pylint for nni_cmd
d7a62f6
@SparkSnail
fix id error
e259d10
@SparkSnail
Merge pull request #212 from microsoft/master
4997295 
merge master
@SparkSnail
Merge pull request #213 from microsoft/master
c037a7c 
merge master
@SparkSnail
Merge pull request #214 from microsoft/master
7620e7c 
merge master
@SparkSnail
Merge pull request #215 from microsoft/master
d16dbe9 
Filter prune algo implementation (microsoft#1655)
@SparkSnail
Merge pull request #216 from microsoft/master
9ce751d 
merge master
@SparkSnail
Merge pull request #217 from microsoft/master
a0846f2 
merge master
@SparkSnail
Merge pull request #218 from microsoft/master
cd3a912 
merge master
@SparkSnail
Merge pull request #219 from microsoft/master
32efaa3 
merge master
@SparkSnail
Merge pull request #220 from microsoft/master
543239c 
merge master
@SparkSnail
Merge pull request #221 from microsoft/master
36e6e35 
merge master
@SparkSnail
Merge pull request #222 from microsoft/master
f9ee589 
merge master
@SparkSnail
Merge pull request #223 from microsoft/master
b9a7a95 
merge master
@SparkSnail
Merge pull request #224 from microsoft/master
1a5c017 
merge master
@SparkSnail
Merge pull request #225 from microsoft/master
392460a 
merge master
@SparkSnail
Merge pull request #226 from microsoft/master
9bafa4c 
Support monitor mode when creating or resuming a new experiment (microsoft#1933)
@SparkSnail
Merge pull request #227 from microsoft/master
c23b807 
Add test for documentation build (microsoft#1924)
@SparkSnail
Merge pull request #228 from microsoft/master
4132f62 
fix pipeline status badge (microsoft#1942)
@SparkSnail
Merge pull request #229 from microsoft/master
4f66d0c 
merge master
@SparkSnail
Merge pull request #230 from microsoft/master
129c4a5 
merge master
QuanluZhang
QuanluZhang reviewed May 13, 2020
View reviewed changes
...anager/training_service/kubernetes/frameworkcontroller/frameworkcontrollerTrainingService.ts
}
const azureKubeflowClusterConfig: FrameworkControllerClusterConfigAzure = <FrameworkControllerClusterConfigAzure>this.fcClusterConfig;
return await this.uploadFolderToAzureStorage(srcDirectory, destDirectory, azureKubeflowClusterConfig.uploadRetryCount);
} else if (this.fcClusterConfig.storage === 'nfs' || this.fcClusterConfig.storage === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or the storage is undefined? This should be an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In original design, the storage field is optional, if it is not set, the default value is nfs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but when storage is undefined, there is no config for storage, how do you mount it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is supposed to be check in NNICTL side, if users didn't set storage field, nnictl should check if usres set nfs configuration.

@QuanluZhang
Copy link
Contributor

@SparkSnail should also verify the logic of code dir in DLTS training service, and update the logic if needed.

@QuanluZhang
Copy link
Contributor

would also better to make sure IT is tested for this pr

@QuanluZhang
Copy link
Contributor

for different training services, there should be exactly the same directory structure for trial working directory. suggest to use a folder named code under the working directory to store user code. The advantage of this way is removing possible name conflict.

@SparkSnail
Copy link
Contributor Author

for different training services, there should be exactly the same directory structure for trial working directory. suggest to use a folder named code under the working directory to store user code. The advantage of this way is removing possible name conflict.

Sure, there are two place to store code for a trial, one path is the common folder for all trials under experiment folder, the path is {experimentId}/nni-code, another path is under trial's working folder, the path is {experimentId}/{trialId}. do you mean adding a code folder under {experimentId}/{trialId} path? do not store run.sh together with code files? then the real working directory will become {experimentId}/{trialId}/code, not {experimentId}/{trialId}.

@SparkSnail
Copy link
Contributor Author

@SparkSnail should also verify the logic of code dir in DLTS training service, and update the logic if needed.

Yes, the trial jobs in DLTS share same codeDir folder, and also need to be refactored.

@QuanluZhang QuanluZhang requested a review from squirrelsc May 19, 2020 00:37
@QuanluZhang
Copy link
Contributor

for different training services, there should be exactly the same directory structure for trial working directory. suggest to use a folder named code under the working directory to store user code. The advantage of this way is removing possible name conflict.

Sure, there are two place to store code for a trial, one path is the common folder for all trials under experiment folder, the path is {experimentId}/nni-code, another path is under trial's working folder, the path is {experimentId}/{trialId}. do you mean adding a code folder under {experimentId}/{trialId} path? do not store run.sh together with code files? then the real working directory will become {experimentId}/{trialId}/code, not {experimentId}/{trialId}.

We can leave this refactor to the next pr.

squirrelsc
squirrelsc reviewed May 19, 2020
View reviewed changes
src/nni_manager/training_service/remote_machine/remoteMachineTrainingService.ts Outdated
for (const [rmMeta, executorManager] of this.machineExecutorManagerMap.entries()) {
const executor: ShellExecutor = await executorManager.getAvailableExecutor();
if (executor !== undefined) {
await executor.createFolder(this.remoteExpCodeDir);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line can also put in async promise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

@squirrelsc
Copy link
Member

Please explain what's changes on folders in PR description.

@SparkSnail SparkSnail merged commit d7456c1 into microsoft:master May 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@QuanluZhang QuanluZhang QuanluZhang approved these changes

@squirrelsc squirrelsc squirrelsc approved these changes

@chicm-ms chicm-ms chicm-ms approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SparkSnail @QuanluZhang @squirrelsc @chicm-ms