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

Modified CUDA easyblock to support wrapper creation #758

Merged
merged 12 commits into from Feb 17, 2016

Conversation

damianam
Copy link
Member

@damianam damianam commented Dec 2, 2015

This PR modifies the CUDA easyblock to support the optional creation of nvcc wrappers. It adds two extra boolean options for easyconfigs: generate_intel_wrapper and generate_gcc_wrapper. The first one will create a wrapper invcc. The second a wrapper called gnvcc.

This allows to simply tell users "Use invcc to have icpc as a host compiler".

This is a convenience fix that doesn't modify the default behaviour or installation.

It will be followed by another PR in the easyconfigs repo, with an example for iccifort+CUDA.

UPDATE: The naming of the wrappers and how they are created has changed significantly. Take a look at the development of the PR.

@hpcugentbot
Copy link
Contributor

Automatic reply from Jenkins: Can I test this?

"""
import os
import stat
import shutil
Copy link
Member

Choose a reason for hiding this comment

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

@damianam: we try to keep these alphabetical, please reorder?

@boegel
Copy link
Member

boegel commented Dec 2, 2015

Jenkins: ok to test

@boegel
Copy link
Member

boegel commented Dec 2, 2015

@damianam: thanks for the contribution! Please take a look at the remarks, and push an extra commit to your branch to resolve them, let us know if you need any help (you know where to find me ;-)).

@boegel boegel added this to the v2.5.0 milestone Dec 2, 2015
@hpcugentbot
Copy link
Contributor

Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1411/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@damianam
Copy link
Member Author

damianam commented Dec 4, 2015

I have applied the suggested changes from this PR and easybuilders/easybuild-easyconfigs#2177. The naming of the host compiler is now provided in the easyconfig, since taking it directly from the framework didn't work (COMPILER_CXX is None, and COMPILER_CUDA_UNIQUE_OPTION_MAP provides an unresolved string 'ccbin="%(CXX_base)s"', since CXX_base is not defined).

It is possible now to create a wrapper and give it a host compiler in the easyconfig. Multiple wrappers in a single installation are not supported in the current PR, since in our scheme we will always have CUDA+iccifort, or CUDA+gcc, or CUDA+pgi. In other words, always paired.

@hpcugentbot
Copy link
Contributor

Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1428/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link
Contributor

Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1432/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.


txt = super(EB_CUDA, self).make_module_extra()

txt += self.module_generator.set_environment('CUDA_HOME', self.installdir)
Copy link
Member

Choose a reason for hiding this comment

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

@damianam: this is already there (see method above)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, but for some reason it is not working. We should keep the discussion here, instead of the one in easybuilders/easybuild-easyconfigs#2177 since this is an easyblock issue. In any case, why use guesses instead of setting these PATHs manually to well known directories?

Copy link
Member

Choose a reason for hiding this comment

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

@damianam: the guesses are checked, in the sense that eb will check whether the directory actually exists before including the corresponding statement in the module file.

Additionally, via guesses we'll use prepend-path rather than setenv.

For $CUDA_HOME in particular, I'm not sure if prepend-path makes sense at all. Probably not...

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it does. CUDA_PATH is not set either in the module. Could it be that prepend-path doesn't work when passing an empty path in the guesses.update? PATH, LD_LIBRARY_PATH and CPATH are defined correctly

Copy link
Member

Choose a reason for hiding this comment

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

OK, I traced not having $CUDA_HOME or $CUDA_PATH being defined down to a bug in the framework.
It has been there for a while (March 2014), but luckily only affects CUDA.

I'll get that fixed as a last-minute thing in the framework, but this PR needs a bit more love before it can go in, so it'll have to be post v2.5.0.

Copy link
Member

Choose a reason for hiding this comment

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

Bug that causes the issue with $CUDA_HOME is fixed in easybuilders/easybuild-framework#1519

Copy link
Member Author

Choose a reason for hiding this comment

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

@boegel I've made a couple of small changes in the easyblock and easyconfig, so now you can specify a list of host compilers, and the easyblock will create the appropriate wrappers based on that.

@hpcugentbot
Copy link
Contributor

Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1602/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@boegel
Copy link
Member

boegel commented Jan 16, 2016

@damianam: you'll need to sync this with current develop, GitHub has detected conflicts and so this can't be merged in as is; let me know if you need help with that

Damian Alvarez added 3 commits January 18, 2016 15:45
…er changes in the original EB repo. Update of cuda.py merging changes in the main repo and the ones on the PR for creating a wrapper.
…easyblocks into updated_cuda

Conflicts:
	easybuild/easyblocks/c/cuda.py
@boegel boegel removed this from the v2.6.0 milestone Jan 23, 2016
@damianam
Copy link
Member Author

@boegel I rebased my repo to match the new main develop branch, but it seems I did something wrong :-|. I'll try to fix it before the user meeting.

@damianam
Copy link
Member Author

@boegel I think it is fixed now. I'll do the same with easybuilders/easybuild-easyconfigs#2177 after lunch

@hpcugentbot
Copy link
Contributor

Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1638/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@fgeorgatos
Copy link
Collaborator

I Could help this a bit during EB meeting, just ask. F.

On Tuesday, January 26, 2016, HPC Ugent build bot notifications@github.com
wrote:

Easyblocks unit test suite PASSed (see
https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1638/console
for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel
https://github.com/boegel if you're not sure what to do.


Reply to this email directly or view it on GitHub
#758 (comment)
.

echo "sysadmin know better bash than english"|sed s/min/mins/
| sed 's/better bash/bash better/' # signal detected in a CERN forum

def create_wrapper(wrapper_name,wrapper_comp):
wrapper_f = "%s/bin/%s" % (self.installdir, wrapper_name)
write_file(wrapper_f, wrapper % wrapper_comp)
adjust_permissions(wrapper_f, stat.S_IXUSR|stat.S_IRUSR|stat.S_IXGRP|stat.S_IRGRP|stat.S_IXOTH|stat.S_IROTH)
Copy link
Member

Choose a reason for hiding this comment

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

wrong indentation (tabs involved)?

@damianam
Copy link
Member Author

@boegel I fixed the style, lack of docstrings, etc.

@hpcugentbot
Copy link
Contributor

Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1645/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@damianam
Copy link
Member Author

ping

@boegel
Copy link
Member

boegel commented Feb 16, 2016

@damianam: thanks for the ping, need to fix conflict with current develop first, cfr. damianam#2

Damian Alvarez added 2 commits February 17, 2016 13:48
@hpcugentbot
Copy link
Contributor

Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1671/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link
Contributor

Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1672/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@damianam
Copy link
Member Author

@boegel done

@boegel
Copy link
Member

boegel commented Feb 17, 2016

Tested with all existing CUDA easyconfigs and the new one from easybuilders/easybuild-easyconfigs#2177, good to go, thanks @damianam!

boegel added a commit that referenced this pull request Feb 17, 2016
Modified CUDA easyblock to support wrapper creation
@boegel boegel merged commit fa247e3 into easybuilders:develop Feb 17, 2016
@damianam damianam deleted the updated_cuda branch July 12, 2017 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants