Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow amending easyconfig parameters which are not the default #3651

Merged
merged 5 commits into from
May 3, 2021

Conversation

Flamefire
Copy link
Contributor

Overwriting parameters which are not in the EC and not one of a few standard ones is currently impossible.
However it is useful when e.g. EasyBlocks introduce new params where the user wants to overwrite the default value without changing the EC manually.
This change allows this by simply appending the new parameter when it is not in the default list.

Overwriting parameters which are not in the EC and not one of a few standard ones is currently impossible.
However it is useful when e.g. EasyBlocks introduce new params where the user wants to overwrite the default
value without changing the EC manually.
This change allows this by simply appending the new parameter when it is not in the default list.
@Flamefire Flamefire force-pushed the overwriteNonDefaultECParams branch from a5f5a04 to befebc3 Compare April 14, 2021 14:54
We already have this case covered by the variable naming enforcement,
i.e. loading an EC with an unknown parameter not named local_* will
already error out
@easybuilders easybuilders deleted a comment from boegelbot Apr 28, 2021
@boegel boegel added this to the next release (4.3.5?) milestone Apr 28, 2021
@boegel boegel changed the title Allow amending EC params which are not the default Allow amending easyconfig parameters which are not the default Apr 28, 2021
easybuild/framework/easyconfig/tweak.py Outdated Show resolved Hide resolved
specs.update({
'foo': 'bar123'
})
self.assertErrorRegex(EasyBuildError, "Unknown easyconfig parameter: foo",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would think this test is still useful for parameters that are not defined in the easyblock, or is that taken care of somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is taken care of elsewhere. I don't think we can trigger this error anymore at all, as that is specific to using non-default parameters which we allow now.
The EasyBlock instantiation will trigger e.g. "Use of 2 unknown easyconfig parameters detected in test.eb: foo, some_list", see test_fix_deprecated_easyconfigs

Flamefire and others added 2 commits May 3, 2021 08:10
Co-authored-by: Sam Moors <smoors@users.noreply.github.com>
@Flamefire
Copy link
Contributor Author

@smoors Took your suggestion but had to add an is-string check or it will fail for unhashable types (e.g. other dicts) as values. I also reordered the dict and interspersed the comments which IMO is easier to understand. Check if you agree please.

Copy link
Contributor

@smoors smoors left a comment

Choose a reason for hiding this comment

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

lgtm

@smoors
Copy link
Contributor

smoors commented May 3, 2021

Going in, thanks @Flamefire!

@smoors smoors merged commit 4aad383 into easybuilders:develop May 3, 2021
@Flamefire Flamefire deleted the overwriteNonDefaultECParams branch May 3, 2021 09:21
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.

3 participants