Skip to content

enhance update_build_option to return original value + add support for update_build_options#3923

Merged
boegel merged 14 commits intoeasybuilders:developfrom
casparvl:update_build_options_for_easystack
Jan 13, 2022
Merged

enhance update_build_option to return original value + add support for update_build_options#3923
boegel merged 14 commits intoeasybuilders:developfrom
casparvl:update_build_options_for_easystack

Conversation

@casparvl
Copy link
Copy Markdown
Contributor

@casparvl casparvl commented Jan 11, 2022

To be able to define (per-)EasyConfig specific options in an EasyStack file, as discussed here we need to easily be able to set entire dictionaries of new configuration options - and restore them to their original values later.

This PR provides that functionality by

  1. extending update_build_option so that it returns the original value (so it can later be restored)

  2. implementing update_build_options, which takes a dictionary, calls update_build_option on each key-value pair in the dictionary, and collects & returns the original values for each in another dictionary. This allows one to call another update_build_options on the returned dictionary to reset the original values.

Example usage:

# Have a dictionary of dictionaries, each specifying a list of options that need to be changed for that specific EasyConfig
    opts_per_ec = {
        'OpenFOAM-8-foss-2020a': {
            'parallel': '6',
            'robot': True,
        },
        'GROMACS-2020.4-foss-2020a-Python-3.8.2': {
            'debug': True,
            'trace': True,
            'from_pr': '1234',
            'include_easyblocks_from_pr': 123,
            'parallel': 6,
            'robot': True,
        },
    }
...
   old_val_dict = update_build_options(opts_per_ec['OpenFOAM-8-foss-2020a'])
...
   # actually build OpenFOAM-8-foss-202a, now using the options that were set of this specific EasyConfig
...
   # Restore the original configuration, so that the original configuration is used to build any _further_ EasyConfigs
   update_build_options(old_val_dict)
...

Of course, the opts_per_ec would normally not be hardcoded, but will (eventually) be returned by the parse_easystack function if it encounters EasyConfig-specific options in the EasyStack file.

@casparvl
Altered the original update_build_option(key,value) so that it now re…
a8bbbd7 
…turns the original value. This allows us to easily restore it. Also, added an update_build_options function that takes a dictionary and calls update_build_option for each key-value pair in the dictionary
@casparvl casparvl added the easystack Issues and PRs related to easystack files label Jan 11, 2022
boegel
boegel requested changes Jan 11, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

@casparvl We should enhance the existing test for update_build_option in test/framework/config.py to verify the return value too, and we should add a test for update_build_options as well (can be added to the existing test_update_build_option imho)

Comment thread easybuild/tools/config.py Outdated
Comment thread easybuild/tools/config.py Outdated
@easybuilders easybuilders deleted a comment from boegelbot Jan 11, 2022
@boegel boegel added this to the next release (4.5.2?) milestone Jan 11, 2022
@boegel boegel added enhancement and removed easystack Issues and PRs related to easystack files labels Jan 11, 2022
@casparvl
Copy link
Copy Markdown
Contributor Author

casparvl commented Jan 13, 2022

@casparvl We should enhance the existing test for update_build_option in test/framework/config.py to verify the return value too, and we should add a test for update_build_options as well (can be added to the existing test_update_build_option imho)

You're right. I added a test for the returned value. Also, I included a test for update_build_options. I actually made it a seperate one, sorry, overlooked your remark that you wanted to add it in the same test. If you want me to integrate them, we can. But I think it's quite nice to test it seperately, since a test failure will then immediately tell you if it's in the update_build_option (in which case both tests fail), or in the 'logic' (looping over the dictionary, adding the returned values to a dictionary) of update_build_options.

boegel
boegel requested changes Jan 13, 2022
View reviewed changes
Comment thread easybuild/tools/config.py Outdated
Comment thread test/framework/config.py Outdated
Comment thread test/framework/config.py
@casparvl
Copy link
Copy Markdown
Contributor Author

casparvl commented Jan 13, 2022

Hm, I'm looking at the one failing test, but not sure what's wrong there. Seems to be a part of the code that I didn't touch at all, and only in one of the build configurations. A fluke? Can I trigger a rerun of the test suite (in some way more elegant than a recommit?)

@boegel
Copy link
Copy Markdown
Member

boegel commented Jan 13, 2022

Hm, I'm looking at the one failing test, but not sure what's wrong there. Seems to be a part of the code that I didn't touch at all, and only in one of the build configurations. A fluke? Can I trigger a rerun of the test suite (in some way more elegant than a recommit?)

FAIL: test_toy_lock_cleanup_signals (test.framework.toy_build.ToyBuildTest)
Test cleanup of locks after EasyBuild session gets a cancellation signal.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/runner/a1a9cf08297f1c3dc8551fe19465b35dbcecf7ed/lib/python3.5/site-packages/test/framework/toy_build.py", line 3536, in test_toy_lock_cleanup_signals
    self.assertTrue(regex.search(stderr), "Pattern '%s' found in: %s" % (regex.pattern, stderr))
AssertionError: None is not true : Pattern '^WARNING: signal received \(3\), cleaning up locks \(.*software_toy_0.0\)\.\.\.' found in: WARNING: signal received (3), cleaning up locks ()...

Yes, that's a fluke... That test fails occasionally, and it has proven quite difficult to chase it down.

Please open an issue on it. I'll retrigger the tests for this PR.

@casparvl casparvl mentioned this pull request Jan 13, 2022
Open
boegel
boegel approved these changes Jan 13, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel boegel changed the title Altered update function of build options in preparation of EasyConfig specific options in EasyStack files add support for update_build_options + enhance update_build_options to return original value Jan 13, 2022
@boegel boegel merged commit 2bd4f83 into easybuilders:develop Jan 13, 2022
@boegel boegel changed the title add support for update_build_options + enhance update_build_options to return original value add support for update_build_options + enhance update_build_option to return original value Jan 23, 2022
@boegel boegel changed the title add support for update_build_options + enhance update_build_option to return original value enhance update_build_option to return original value + add support for update_build_options Jan 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@boegel boegel boegel approved these changes

Assignees

@boegel boegel

Labels

enhancement

Projects

None yet

Milestone

4.5.2

Development

Successfully merging this pull request may close these issues.

2 participants

@casparvl @boegel