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

Replace parallel by maxparallel #19375

Merged
merged 5 commits into from
Nov 22, 2024
Merged

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Dec 7, 2023

The latter is the new, better option which makes it clear how it works with --parallel passed in the config or commandline.

5.x target of #19374 required by easybuilders/easybuild-framework#4398

Note: CI fails for unrelated reasons and fixing all those ECs is unfeasible, so I'd suggest to bite the bullet and merge this even though CI fails.

@Flamefire
Copy link
Contributor Author

@boegel Resolved conflicts

@boegelbot
Copy link
Collaborator

@Flamefire: Tests failed in GitHub Actions, see https://github.com/easybuilders/easybuild-easyconfigs/actions/runs/7421645944
Output from first failing test suite run:

FAIL: test_pr_CMAKE_BUILD_TYPE (test.easyconfigs.easyconfigs.EasyConfigTest)
Make sure -DCMAKE_BUILD_TYPE is no longer used (replaced by build_type)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/easybuild-easyconfigs/easybuild-easyconfigs/test/easyconfigs/easyconfigs.py", line 1278, in test_pr_CMAKE_BUILD_TYPE
    self.fail('\n'.join(failing_checks))
AssertionError: build_type was set to the default of 'Release'. Omit this to base it on toolchain_opts.debug: kim-api-2.3.0-GCC-11.2.0.eb
build_type was set to the default of 'Release'. Omit this to base it on toolchain_opts.debug: kim-api-2.3.0-GCC-11.3.0.eb

----------------------------------------------------------------------
Ran 8672 tests in 342.400s

FAILED (failures=1)
ERROR: Not all tests were successful

bleep, bloop, I'm just a bot (boegelbot v20200716.01)
Please talk to my owner @boegel if you notice me acting stupid),
or submit a pull request to https://github.com/boegel/boegelbot fix the problem.

@boegel boegel added change EasyBuild-5.0 EasyBuild 5.0 labels Jan 22, 2024
@boegel boegel added this to the 5.0 milestone Jan 22, 2024
@easybuilders easybuilders deleted a comment from boegelbot Jan 22, 2024
@Flamefire
Copy link
Contributor Author

Rebased and updated. Let's see what CI says ;-)

@boegel
Copy link
Member

boegel commented Oct 8, 2024

@Flamefire We should add a small check in the easyconfig test suite to make sure that no easyconfigs are still setting parallel?

The latter is the new, better option which makes it clear how it works
with `--parallel` passed in the config or commandline.
@Flamefire
Copy link
Contributor Author

@Flamefire We should add a small check in the easyconfig test suite to make sure that no easyconfigs are still setting parallel?

I added a CI check for all deprecated EC parameters.

The setting I mentioned in the confcall that would detect any deprecated use is here:

# os.environ['EASYBUILD_DEPRECATED'] = '10000'

It might be better to enable this instead to check for any deprecated behavior.

However I disabled that in #13257 with the comment

Remove unused setting of EASYBUILD_DEPRECATED which, if in effect, would cause many failures due to deprecated ECs (using old versions) or even using LMod 6 (on CI) is already deprecated

I'm not sure why I determined this as unused back then, so maybe that has changed. If I do enable it the test fails earlier:

  File "/git/easybuild-easyconfigs/test/easyconfigs/easyconfigs.py", line 251, in _get_changed_easyconfigs
    ec = process_easyconfig(ec_file)
  File "/git/easybuild-framework/easybuild/framework/easyconfig/easyconfig.py", line 2147, in process_easyconfig
    raise EasyBuildError("Failed to process easyconfig %s: %s", spec, err.msg, exit_code=exit_code)
easybuild.tools.build_log.EasyBuildError: "Failed to process easyconfig /git/easybuild-easyconfigs/easybuild/easyconfigs/b/Bracken/Bracken-2.7-GCCcore-11.2.0.eb: DEPRECATED (since v5.1) functionality used: Easyconfig parameter 'parallel' is deprecated, use 'maxparallel' instead.; see https://docs.easybuild.io/deprecated-functionality/ for more information"

Copy link
Contributor

@Micket Micket left a comment

Choose a reason for hiding this comment

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

So, I tried to follow the discussion across multiple PRs, maybe I'm missing something, but this should just be good to go right? I browsed through the changes and I think they look fine. The extra check looks OK to me. Regardless of the changes in framework or easyblocks, this is just a necessary and uncontroversial merge right?

@Flamefire
Copy link
Contributor Author

Yes I think I addressed all comments of @boegel

I can open another PR enabling $EASYBUILD_DEPRECATED for 5.0x which is currently commented out (in 5.0x and develop) and I think we really do not want to use any deprecated behavior in 5.0x. That is mostly a generalization of the check added in 93ed7fe

I'm not fully sure which one we want: The test_pr_deprecated_ec_params catches only deprecated EC params while $EASYBUILD_DEPRECATED can catch any deprecated usage. On the other hand the test method reports failures for all (changed) ECs in a single error while $EASYBUILD_DEPRECATED would error and stop on the first failure.

In any case I think we should make up our mind for 5.0x and either enable $EASYBUILD_DEPRECATED or remove the commented part.

And of course we should merge this ASAP to avoid having to keep fixing things not caught by current CI

@Micket
Copy link
Contributor

Micket commented Nov 22, 2024

Sounds good to me. I see no objections here so I will push forward and we can follow up if there is something.

@Micket Micket merged commit 0e07a83 into easybuilders:5.0.x Nov 22, 2024
7 checks passed
@Flamefire Flamefire deleted the replace_parallel-5 branch November 22, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants