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

added OpenMPI support for sherpa #3108

Closed
wants to merge 1 commit into from

Conversation

pmillet
Copy link
Contributor

@pmillet pmillet commented Jun 18, 2017

Adds openmpi support to Sherpa as requested in cms-sw/cmssw#18174

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @pmillet for branch IB/CMSSW_9_2_X/gcc700.

@cmsbuild, @smuzaffar, @mrodozov, @iahmad-khan, @davidlange6 can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.

@@ -0,0 +1,25 @@
### RPM external autotools-toolfile 1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do yo need this toolfile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this toolfile from the 53X PR. I can see if it works without.

@@ -0,0 +1,29 @@
--- a/config/orte_check_lsf.m4 2017-05-10 17:40:48.000000000 +0200
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this patch upstreamed? If yes, please make sure that SPEC file or/and filename contains revision information. This will help us track the need of this patch next time we bump OpenMPI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got the patch from open-mpi/ompi#3546
openmpi.spec has the tarball version for openmpi included
the patch is has the version number in the name
is this the revision information you had in mind?

<lib name="mca_common_sm"/>
<lib name="mpi"/>
<lib name="mpi_cxx"/>
<lib name="mpi_f77"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really need all of this? Especially Fortran 77 and 90 interfaces? Try to keep it bare minimum and disable what's not needed/relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above this is copied from the 53X PR. I'll see if it works without.

@@ -15,6 +15,18 @@ Requires: openloops
%endif # isamd64
%endif # islinux

%if "%{?cms_cxx:set}" != "set"
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove all 3, never used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidlt
Copy link
Contributor

davidlt commented Jun 19, 2017

Could you also tell us more about the testing (incl. it's environment) you have done?

@pmillet
Copy link
Contributor Author

pmillet commented Jun 19, 2017

I tested building sherpa using ARCH=slc6_amd64_gcc700 with CMSSW=CMSSW_9_2_ROOT6_X (this also builds openmpi).
Once this was working I added these built versions of sherpa and openmpi to a recent nightly build (CMSSW_9_2_ROOT6_X_2017-06-14-2300) and tested if I could use openmpi to do the cross-section integration in sherpa with an example card.

@smuzaffar
Copy link
Contributor

cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 19, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20723/console

@@ -3,7 +3,7 @@
%define branch cms/v%realversion
%define github_user cms-externals
Source: git+https://github.com/%github_user/%{n}.git?obj=%{branch}/%{tag}&export=%{n}-%{realversion}&output=/%{n}-%{realversion}-%{tag}.tgz
Requires: hepmc lhapdf blackhat sqlite fastjet openssl scons python
Requires: hepmc lhapdf blackhat sqlite fastjet openssl scons python openmpi
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add this dependency for slc*_amd64_gcc700

@@ -35,6 +35,7 @@ cat << \EOF_TOOLFILE >%i/etc/scram.d/sherpa.xml
<use name="blackhat"/>
<use name="fastjet"/>
<use name="sqlite"/>
<use name="openmpi/"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

this should also be added if sherpa with build with openmpi

@davidlt
Copy link
Contributor

davidlt commented Jun 19, 2017

After all the comments could you redo the PR for DEVEL IBs (gcc630next branch IIRC)? There is no need gcc700 branch (just re-gen the build scripts).

@davidlange6
Copy link
Contributor

davidlange6 commented Jun 19, 2017 via email

@pmillet
Copy link
Contributor Author

pmillet commented Jun 19, 2017

the openmpi patch needs the updated automake
iirc it was only updated in the gcc700 branch, right?

@davidlt
Copy link
Contributor

davidlt commented Jun 19, 2017

No, the build script generator clearly states that automake 1.12.2 is just fine. As discussed in the tickets, just force regeneration of all build scripts.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@pmillet
Copy link
Contributor Author

pmillet commented Jun 21, 2017

I got a new branch with requested fixes ready. Should I open a new PR for a different branch (e.g. gcc630next) ?

@smuzaffar
Copy link
Contributor

@pmillet , yes please.

@pmillet
Copy link
Contributor Author

pmillet commented Jun 21, 2017

superseded by #3114

@pmillet pmillet closed this Jun 21, 2017
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.

None yet

5 participants