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

Add shared storage integration test #3455

Merged
merged 52 commits into from
Jun 30, 2021

Conversation

acured
Copy link
Contributor

@acured acured commented Mar 17, 2021

Blocked by NFS mounting issue (maybe port issue). Needs Quanlu's help.

@SparkSnail SparkSnail requested a review from J-shang March 19, 2021 09:32
@acured acured marked this pull request as draft March 19, 2021 09:35
@acured acured force-pushed the addSharedStorageTest branch 2 times, most recently from 63e66d3 to 1b73e96 Compare April 8, 2021 02:34
@kvartet kvartet mentioned this pull request Apr 26, 2021
49 tasks
@acured acured marked this pull request as ready for review May 14, 2021 09:07
@acured acured changed the title [No need to review] add shared storage test Add shared storage integration test May 14, 2021
@ultmaster ultmaster requested a review from SparkSnail May 17, 2021 07:05
@@ -53,6 +53,18 @@ def update_training_service_config(config, training_service, config_file_path):
it_ts_config[training_service]['trial']['codeDir'] = containerCodeDir
it_ts_config[training_service]['trial']['command'] = 'cd {0} && {1}'.format(containerCodeDir, config['trial']['command'])

if training_service == 'remote':
testcase_config = get_yml_content(args.nni_source_dir + config_file_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

where we define args?

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

def match_remoteConfig(test_case_config):
trainingservice_config = get_yml_content(os.path.join('config', 'training_service.yml'))
trainingservice_config_reuse_value = str(trainingservice_config['remote']['remoteConfig']['reuse']).lower()
testcase_config = get_yml_content(args.nni_source_dir + test_case_config['configFile'])
Copy link
Contributor

Choose a reason for hiding this comment

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

args defination

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

@@ -258,6 +279,11 @@ def run(args):
print('skipped {}, training service {} not match [{}]'.format(
name, args.ts, test_case_config['trainingService']))
continue

if not match_remoteConfig(test_case_config):
Copy link
Contributor

Choose a reason for hiding this comment

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

a little strange if we do not test remote but we need to judge match_remoteConfig, maybe put these lines under line 288 is better.

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

@@ -0,0 +1,24 @@
"""
A deep MNIST classifier using convolutional layers.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't use the nni example, I think create a new folder to handle trial code and config parallel with test/config/examples is better.

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

Copy link
Contributor

@J-shang J-shang left a comment

Choose a reason for hiding this comment

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

I think we need test this in the pipeline before merge.

@acured acured force-pushed the addSharedStorageTest branch 2 times, most recently from fb96d8e to 045cb62 Compare May 27, 2021 10:11
@ultmaster ultmaster marked this pull request as draft May 27, 2021 10:18
@acured acured force-pushed the addSharedStorageTest branch 2 times, most recently from 01a91dd to 0613e57 Compare June 21, 2021 04:42
@acured acured marked this pull request as ready for review June 23, 2021 05:34
@J-shang J-shang merged commit 8f01c77 into microsoft:master Jun 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants