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

{compiler}[GCCcore/9.3.0] Add NVHPC v20.9, v20.11 and default to accept EULA #11920

Merged
merged 7 commits into from
Jan 31, 2021

Conversation

AndiH
Copy link
Contributor

@AndiH AndiH commented Dec 21, 2020

This pull request adds NVHPC 20.11, starting off of #11694.

Additionally, I got approval from @samcmill to default-accept the EULA and have, hence, uncommented the source_urls definition. I added a comment relating to the EULA, though.

Copy link
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

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

@AndiH could you also add the source_urls and comment about the EULA in v20.11 to the easyconfig of v20.9?
I also have a minor suggestion to reduce duplicate code in the easyconfig and make it more readable. Please see below

easybuild/easyconfigs/n/NVHPC/NVHPC-20.11.eb Outdated Show resolved Hide resolved
easybuild/easyconfigs/n/NVHPC/NVHPC-20.9.eb Outdated Show resolved Hide resolved
@lexming lexming added the update label Dec 22, 2020
@lexming lexming added this to the 4.x milestone Dec 22, 2020
@lexming lexming changed the title Add NVHPC 20.11, Accept EULA {compiler} [NVHPC/20.9, NVHPC/20.11] Add v20.11 and default to accept EULA Dec 22, 2020
@lexming lexming changed the title {compiler} [NVHPC/20.9, NVHPC/20.11] Add v20.11 and default to accept EULA {compiler}[GCCcore/9.3.0] Add NVHPC v20.9, v20.11 and default to accept EULA Dec 22, 2020
@boegel
Copy link
Member

boegel commented Dec 31, 2020

@AndiH W.r.t. the EULA, we could enhance the NVHPC easyblock to use the EasyBlock.check_accepted_eula method, see easybuilders/easybuild-framework#3535?

@AndiH
Copy link
Contributor Author

AndiH commented Jan 3, 2021

@boegel We could! But since we got the general ok, I'd suggest to rather always accept it!

@verdurin
Copy link
Member

verdurin commented Jan 4, 2021

@AndiH any update on the suggestions from @lexming ?

@AndiH
Copy link
Contributor Author

AndiH commented Jan 4, 2021

@verdurin @lexming I think it makes it less readable (because more indirection), but easier to update in the future. I'll gladly implement. Give me a few days, though; I'm currently officially on parental leave :) .

@AndiH
Copy link
Contributor Author

AndiH commented Jan 11, 2021

I've added the following changes

  • implemented @lexming's suggestions regarding restructuring of source filenames using local_tarball_tmpl
  • backported these changes plus the EULA default-accept to NVHPC 20.7

Copy link
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

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

@AndiH thanks for updating the PR. I made the changes requested by @boegel to the easyblock of NVHPC (see easybuilders/easybuild-easyblocks#2311) to better handle the acceptance of the EULA.

Since you already checked that the EULA of NVHPC can be automatically accepted by just downloading the sources, we can add accept_eula = True to these easyconfigs to automatize the process. See below

easybuild/easyconfigs/n/NVHPC/NVHPC-20.11.eb Show resolved Hide resolved
easybuild/easyconfigs/n/NVHPC/NVHPC-20.9.eb Show resolved Hide resolved
@boegelbot

This comment has been minimized.

AndiH and others added 2 commits January 30, 2021 16:24
Co-authored-by: Alex Domingo <alex.domingo.toro@vub.be>
Co-authored-by: Alex Domingo <alex.domingo.toro@vub.be>
@AndiH
Copy link
Contributor Author

AndiH commented Jan 30, 2021

Oh, sorry, I didn't recognize there was additional work for me to be done :). It's done now!

@verdurin
Copy link
Member

@boegelbot please test @ generoso

@boegelbot
Copy link
Collaborator

@verdurin: Request for testing this PR well received on generoso

PR test command 'EB_PR=11920 EB_ARGS= /apps/slurm/default/bin/sbatch --job-name test_PR_11920 --ntasks=4 ~/boegelbot/eb_from_pr_upload_generoso.sh' executed!

  • exit code: 0
  • output:
Submitted batch job 13717

Test results coming soon (I hope)...

- notification for comment with ID 770251818 processed

Message to humans: this is just bookkeeping information for me,
it is of no use to you (unless you think I have a bug, which I don't).

@boegelbot
Copy link
Collaborator

Test report by @boegelbot
FAILED
Build succeeded for 0 out of 2 (2 easyconfigs in total)
generoso-x-5 - Linux centos linux 8.2.2004, x86_64, Intel(R) Xeon(R) CPU E5-2667 v3 @ 3.20GHz (haswell), Python 3.6.8
See https://gist.github.com/94a2c0bb6c6e3f7acab19b63cc36dd1e for a full test report.

@lexming
Copy link
Contributor

lexming commented Jan 31, 2021

Test report by @lexming
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in total)
node106.hydra.os - Linux centos linux 7.9.2009, x86_64, Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz, Python 2.7.5
See https://gist.github.com/f8adb4fa839043d16d7df4def48a4fb0 for a full test report.

Copy link
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

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

@AndiH thanks for updating the PR!
@verdurin your test failed because NVHPC requires some --cuda-compute-capabilities to be set
LGTM

@lexming lexming modified the milestones: 4.x, next release (4.3.3?) Jan 31, 2021
@lexming
Copy link
Contributor

lexming commented Jan 31, 2021

Going in, thanks @AndiH !

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.

5 participants