Skip to content

Conversation

@ekouts
Copy link
Contributor

@ekouts ekouts commented Feb 19, 2020

The PR, so far, contains only the function that converts the old configuration to a python json object. It also updates the validation schema and the example of the new configuration, which is based on unittests/resources/settings.py.

The validation schema still lacks support for the variables:

  • job_poll_intervals
  • job_submit_timeout

Closes #1114.

@pep8speaks
Copy link

pep8speaks commented Feb 19, 2020

Hello @ekouts, Thank you for updating!

Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide

Comment last updated at 2020-03-23 17:44:50 UTC

@ekouts ekouts requested a review from victorusu February 19, 2020 17:25
@codecov-io
Copy link

codecov-io commented Feb 19, 2020

Codecov Report

Merging #1186 into master will decrease coverage by 0.68%.
The diff coverage is 6.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1186      +/-   ##
==========================================
- Coverage   92.02%   91.34%   -0.69%     
==========================================
  Files          81       83       +2     
  Lines       11736    12208     +472     
==========================================
+ Hits        10800    11151     +351     
- Misses        936     1057     +121
Impacted Files Coverage Δ
unittests/resources/settings.py 100% <ø> (ø) ⬆️
reframe/core/config.py 50% <6.89%> (-34.49%) ⬇️
unittests/test_policies.py 97.64% <0%> (-1.31%) ⬇️
reframe/core/buildsystems.py 96.75% <0%> (-0.37%) ⬇️
reframe/frontend/executors/__init__.py 97.66% <0%> (-0.36%) ⬇️
reframe/core/pipeline.py 92.3% <0%> (-0.29%) ⬇️
unittests/test_schedulers.py 96.86% <0%> (-0.03%) ⬇️
reframe/core/containers.py 100% <0%> (ø) ⬆️
reframe/core/launchers/local.py 100% <0%> (ø) ⬆️
... and 75 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88ef937...97f8fe0. Read the comment docs.

@ekouts ekouts changed the title [wip][feat] Create a tool to convert from the old to the new configuration syntax [feat] Create a tool to convert from the old to the new configuration syntax Feb 24, 2020
@vkarak
Copy link
Contributor

vkarak commented Mar 9, 2020

@ekouts How your ppretty() function differs from Python's pprint()?

@ekouts
Copy link
Contributor Author

ekouts commented Mar 9, 2020

I tried pprint() with indent=2 but the output didn't look that much like the example. On the other hand if you think it's good enough then I can use this instead. The best I could get is this:

{ 'systems': [ { 'descr': 'Generic example system',
                 'hostnames': ['localhost'],
                 'name': 'generic',
                 'partitions': [ { 'access': [],
                                   'descr': 'Login nodes',
                                   'environs': ['builtin-gcc'],
                                   'launcher': 'local',
                                   'modules': [],
                                   'name': 'login',
                                   'scheduler': 'local'}]},
               { 'descr': 'Fake system for unit tests',
                 'hostnames': ['testsys'],
                 'modules': ['foo/1.0'],
                 'name': 'testsys',
                 'partitions': [ { 'descr': 'Login nodes',
                                   'environs': [ 'PrgEnv-cray',
                                                 'PrgEnv-gnu',
                                                 'builtin-gcc'],
                                   'launcher': 'local',
                                   'name': 'login',
                                   'resources': [],
                                   'scheduler': 'local'},
                                 { 'access': [],
                                   'descr': 'GPU partition',
                                   'environs': ['PrgEnv-gnu', 'builtin-gcc'],
                                   'launcher': 'srun',
                                   'modules': ['foogpu'],
                                   'name': 'gpu',
                                   'resources': [ { 'name': 'gpu',
                                                    'options': [ '--gres=gpu:{num_gpus_per_node}']},
                                                  { 'name': 'datawarp',
                                                    'options': [ '#DW jobdw '
                                                                 'capacity={capacity}',
                                                                 '#DW stage_in '
                                                                 'source={stagein_src}']}],
                                   'scheduler': 'slurm',
                                   'variables': [['FOO_GPU', 'yes']]}],
                 'perflogdir': '.rfm_testing/perflogs',
                 'prefix': '.rfm_testing',
                 'resourcesdir': '.rfm_testing/resources',
                 'variables': [['FOO_CMD', 'foobar']]},
               { 'descr': 'System for checking test dependencies',
                 'hostnames': ['sys\\d+'],
                 'name': 'sys0',
                 'partitions': [ { 'environs': ['e0', 'e1'],
                                   'launcher': 'local',
                                   'name': 'p0',
                                   'scheduler': 'local'},
                                 { 'environs': ['e0', 'e1'],
                                   'launcher': 'local',
                                   'name': 'p1',
                                   'scheduler': 'local'}]}]}

And the output from the one I wrote is this:

{
    'systems': [
        {
            'name': 'generic',
            'descr': 'Generic example system',
            'hostnames': [
                'localhost'
            ],
            'partitions': [
                {
                    'name': 'login',
                    'scheduler': 'local',
                    'modules': [],
                    'access': [],
                    'environs': [
                        'builtin-gcc'
                    ],
                    'descr': 'Login nodes',
                    'launcher': 'local'
                }
            ]
        },
        {
            'name': 'testsys',
            'descr': 'Fake system for unit tests',
            'hostnames': [
                'testsys'
            ],
            'prefix': '.rfm_testing',
            'resourcesdir': '.rfm_testing/resources',
            'perflogdir': '.rfm_testing/perflogs',
            'modules': [
                'foo/1.0'
            ],
            'variables': [
                [
                    'FOO_CMD',
                    'foobar'
                ]
            ],
            'partitions': [
                {
                    'name': 'login',
                    'scheduler': 'local',
                    'resources': [],
                    'environs': [
                        'PrgEnv-cray',
                        'PrgEnv-gnu',
                        'builtin-gcc'
                    ],
                    'descr': 'Login nodes',
                    'launcher': 'local'
                },
                {
                    'name': 'gpu',
                    'scheduler': 'slurm',
                    'modules': [
                        'foogpu'
                    ],
                    'variables': [
                        [
                            'FOO_GPU',
                            'yes'
                        ]
                    ],
                    'resources': [
                        {
                            'name': 'gpu',
                            'options': [
                                '--gres=gpu:{num_gpus_per_node}'
                            ]
                        },
                        {
                            'name': 'datawarp',
                            'options': [
                                '#DW jobdw capacity={capacity}',
                                '#DW stage_in source={stagein_src}'
                            ]
                        }
                    ],
                    'access': [],
                    'environs': [
                        'PrgEnv-gnu',
                        'builtin-gcc'
                    ],
                    'descr': 'GPU partition',
                    'launcher': 'srun'
                }
            ]
        },
        {
            'name': 'sys0',
            'descr': 'System for checking test dependencies',
            'hostnames': [
                'sys\\d+'
            ],
            'partitions': [
                {
                    'name': 'p0',
                    'scheduler': 'local',
                    'environs': [
                        'e0',
                        'e1'
                    ],
                    'launcher': 'local'
                },
                {
                    'name': 'p1',
                    'scheduler': 'local',
                    'environs': [
                        'e0',
                        'e1'
                    ],
                    'launcher': 'local'
                }
            ]
        }
    ],
}

@vkarak
Copy link
Contributor

vkarak commented Mar 9, 2020

@ekouts Way better your formatting. I think we should have this as a separate function in utility. I will do my full review shortly.

@ekouts
Copy link
Contributor Author

ekouts commented Mar 9, 2020

I think we should have this as a separate function in utility.

Yes it makes sense because it's more general and actually handles tuples as well, that are not part of the configuration as far as I could tell.

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

As discussed, move the ppretty() function to the utility module and add a unit test. It'd be good also to have a unit test for the old config conversion. Copy the unittests/resources/settings.py to unittests/resources/settings_old_style.py, load it, convert it and then validate it against the schema.

@vkarak
Copy link
Contributor

vkarak commented Mar 12, 2020

@ekouts Don't write a unit tests for the conversion. I will include one in my upcoming PR for the new configuration mechanism. Just move your pretty print function to a utility and unit test that one only.

@vkarak
Copy link
Contributor

vkarak commented Mar 12, 2020

Moving over to the next sprint.

ekouts added 4 commits March 13, 2020 16:32
 - Fix container platform handling
 - Fix variables in environments
 - Delete type attribute from environments
Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks @ekouts !

@vkarak vkarak changed the title [feat] Create a tool to convert from the old to the new configuration syntax [feat] Add a tool to convert from the old to new configuration syntax Mar 23, 2020
@vkarak vkarak changed the title [feat] Add a tool to convert from the old to new configuration syntax [feat] Add a tool to convert from the old to the new configuration syntax Mar 23, 2020
@vkarak vkarak merged commit 7fff3ca into reframe-hpc:master Mar 23, 2020
@ekouts ekouts deleted the feat/json-config-tool branch March 24, 2020 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create a tool to convert from the old to the new configuration syntax

4 participants