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.

Sign up for GitHub

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

Jump to bottom

Deprecate failure to resolve a template value #3285

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Deprecate failure to resolve a template value #3285

wants to merge 6 commits into from

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Apr 17, 2020 •
edited
Loading

A failure in template resolving is currently hidden in the log although it is likely a bug in the code (either EasyBuild, EasyBlock or EasyConfig)
This may lead to cryptic error messages later on or misbehave silently which is even worse.
To not break builds completely deprecate continuing for now and promote to a hard error later. The message is now highly visible and existing cases can be fixed until the next release.

I did an extensive run during development of #3129 with this issue promoted to a hard failure and did not encounter a single instance. So I think there should be no issues with existing ECs (or very few)

@boegel To have enough time to check for existing issues until next release I recommend merging this soon.

@Flamefire Flamefire mentioned this pull request Apr 17, 2020
Merged
@Flamefire
Copy link
Contributor Author

Flamefire commented Apr 17, 2020 •
edited
Loading

Fleshed out some changes into #3287, there are still some logic issues that would need resolving:

  • the cfg['ext_list']['options'] should not be resolved by cfg as this would be using the wrong values
  • More general: fetch_extension_sources should run after creating the instances to avoid duplicating the logic of updating the template dict values which has the potential of becoming out of sync
  • One failure is with the python multidep as for some reason the dependencies are flattened but resolved but resolval is partial as pyver isn't set yet. This mirrors in short/long_mod_name. Not sure if this is a problem but it may.
  • final failure is due to test yep files differing from the ec files (yep files use some replacement in yep, while ec use %(name)s replacements). IMO this is a mistake and the files should be equivalent (before templating in EB)

@akesandgren
Copy link
Contributor

@Flamefire conflict resolution and merge with develop please.

@Flamefire Flamefire mentioned this pull request May 4, 2020
Closed
@Flamefire Flamefire force-pushed the deprecate_template_failure branch from 1b74197 to e46c69a Compare May 4, 2020 07:37
@easybuilders easybuilders deleted a comment from boegelbot May 19, 2020
@easybuilders easybuilders deleted a comment from boegelbot May 19, 2020
@boegel boegel added the change label May 19, 2020
@boegel boegel added this to the release after 4.2.1 (4.2.2?) milestone May 19, 2020
@easybuilders easybuilders deleted a comment from boegelbot Oct 14, 2020
@boegel boegel modified the milestones: next release (4.3.1), 4.x Oct 14, 2020
@Flamefire Flamefire force-pushed the deprecate_template_failure branch 2 times, most recently from bb6f129 to edb4478 Compare July 5, 2022 11:16
@easybuilders easybuilders deleted a comment from boegelbot Jul 6, 2022
@Flamefire
Copy link
Contributor Author

@akesandgren Done, so this is ready

@akesandgren
Copy link
Contributor

This one's too complicated for me, someone else that feels confident around that code needs to do the review.

@boegel
Copy link
Member

boegel commented Aug 3, 2022

@Flamefire Can you do a sync with develop here too, so the correct set of configurations for the test suite runs is used in CI?

@Flamefire
Copy link
Contributor Author

Rebased.

@Flamefire Flamefire force-pushed the deprecate_template_failure branch 2 times, most recently from 6996186 to 89e1d0b Compare May 11, 2023 16:53
@boegel boegel modified the milestones: 4.x, 5.0 Oct 16, 2023
@boegel boegel added the EasyBuild-5.0 EasyBuild 5.0 label Oct 16, 2023
@boegel
Copy link
Member

boegel commented Nov 13, 2023

@Flamefire Can this be re-targeted to the 5.0.x branch?

@Flamefire
Copy link
Contributor Author

@Flamefire Can this be re-targeted to the 5.0.x branch?

Sure can do. Do we want the deprecation there or the failure? For me the deprecation would be good to have "yesterday" and failure in 5.x

@boegel
Copy link
Member

boegel commented Dec 6, 2023

@Flamefire Can this be re-targeted to the 5.0.x branch?

Sure can do. Do we want the deprecation there or the failure? For me the deprecation would be good to have "yesterday" and failure in 5.x

That works for me, but only if we can ensure that not everyone who uses EasyBuild 4.9.0 will be exposed to deprecation warnings because we didn't do the necessary work to try and avoid them.

For EasyBuild 5.0 we have an opportunity to make it a hard failure, so let's try doing that.
Worst case we make it just a warning again later if it turns out that was a bit ambitious...

@Flamefire
Copy link
Contributor Author

That works for me, but only if we can ensure that not everyone who uses EasyBuild 4.9.0 will be exposed to deprecation warnings because we didn't do the necessary work to try and avoid them.

I already did some work for that all over the place in this PR.
Yes there might be some fallout but the only ways to find those is to do extensive test reinstalls with this turned to an error or just let it be as-is: A deprecation that this will be an error in 5.0 (as you seem to agree) and users can ignore this or better: report this. So I don't see it as necessarily bad if it bothers someone ;-)

As mentioned it would have been good to have the deprecation in yesterday such that we could have had time to find common remaining issues.

For EasyBuild 5.0 we have an opportunity to make it a hard failure, so let's try doing that. Worst case we make it just a warning again later if it turns out that was a bit ambitious...

Might even need to deprecate it targetting 5.1 instead of 5.0 and/or allow this to pass via a switch. IIRC we have an option to allow e.g. deprecated toolchains which by default would error, don't we? We could use the same here for a version or 2. But the earlier we get this tested a lot the better.

@Flamefire
Copy link
Contributor Author

I found the option: --silence-deprecation-warnings=resolve-templates will now silence the warning and even avoid the failure if this gets merged into 5.0 as-is. What do you say @boegel , is this enough now?

@Flamefire
Copy link
Contributor Author

PR for 5.x (with hard error): #4516

@Flamefire
Deprecate failure to resolve a template value
60cd97b 
A failure in template resolving is currently hidden in the log although
it is likely due to a bug in the code (either EasyBuild, EasyBlock or EasyConfig)
This may lead to cryptic error messages later on or misbehave silently
which is even worse.
To not break builds completely deprecate continuing for now and promote
to a hard error later.
@Flamefire
Avoid uneccessary template resolving
2729a78 
If we only want the `len` of some list we don't need to resolve the templates.
We cannot resolve templates in collect_exts_file_info like `installdir` present in e.g.
`installcmd` of the extension options.
@Flamefire
Allow unresolved templates in _finalize_dependencies, extension opt…
6a488be 
…ions & `asdict`

Some dependencies may use templates referring to other dependencies
which cannot yet be resolved. Hence add an opt-out to the enforcment
of resolved templates via `expect_resolved=False` and use that.

This allows use of `%(installdir)s` or `%(ext_name)s` and similar in the
options.
@Flamefire
Add option to silence the deprecation about failure to resolve a temp…
45fbad9 
…late

Useful workaround if there are still some missing fixes.
@Flamefire
Fix test comparing YEB and EB files
e29bf48 
Use the same templates in the compared EC and YEB files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
change EasyBuild-5.0 EasyBuild 5.0
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Flamefire @akesandgren @boegel