[wip] charmpp : fix build prefix, introduce new property charmarch by s-sajid-ali · Pull Request #15666 · spack/spack

s-sajid-ali · 2020-03-25T02:20:24Z

Rationale : As explained in #15186 (comment)

I had to add -lpthread as without this I was getting errors regarding undefined symbols. I temporarily commented out the backend=mpi provides as this was causing concretization errors (when building with +openmpi/pmix as specified in namd instructions). What would be the long term fix for this ?

Successfully built spec documented here.

Sanity check :

[sajid@xrmlite ~]$ cd $(spack location -i /5pq)/bin
[sajid@xrmlite bin]$ ./charmrun ./namd2

Running on 1 processors:  ./namd2
charmrun>  /usr/bin/setarch x86_64 -R  mpirun -np 1  ./namd2
This version of Spack (openmpi ~legacylaunchers schedulers=slurm)
is installed without the mpiexec/mpirun commands to prevent
unintended performance issues. See https://github.com/spack/spack/pull/10340
for more details.
^C

Requesting @matthiasdiener for review !

…_ucx

var/spack/repos/builtin/packages/charmpp/package.py

matthiasdiener · 2020-03-25T02:48:53Z

Looks good imo. I'm not sure about the namd/pthreads issue, but this might be better in a separate PR?

Regarding the backend=mpi provides, I think this is not critical functionality and can be commented out for now. I'm not sure what the long-term fix for this is.

s-sajid-ali · 2020-03-25T02:54:32Z

Any update on #15186 (comment) ?

I'll fix the flake8 issues soon.

var/spack/repos/builtin/packages/charmpp/package.py

matthiasdiener · 2020-03-25T15:44:03Z

var/spack/repos/builtin/packages/charmpp/package.py

+    # https://github.com/UIUC-PPL/charm/issues/2477
+    variant(
+        "ucx-pmi",
+        default="simplePMI",


As far as I know, PMIx is the version with the best support in Charm++ currently, maybe this could be the default.

For both ucx and non-ucx installs ?

I think PMI is only used when building with ucx, but @nitbhat may be able to confirm.

All of gni, ofi and ucx use PMI.

matthiasdiener · 2020-03-25T15:47:46Z

Also pinging @nitbhat for review.

s-sajid-ali · 2020-03-25T15:59:00Z

Looks like the +pthreads variant is not supported anymore in the latest release. When was it deprecated ?

matthiasdiener · 2020-03-25T16:02:47Z

Looks like the +pthreads variant is not supported anymore in the latest release. When was it deprecated ?

As far as I know, it is still supported. Do you get an error when building with it?

scheibelp · 2020-05-02T02:18:12Z

If spack users install namd+charmpp then the mpi implementation gets loaded into the environment due to fftw+mpi dependency. Would it be a good idea to add a run only mpi dependency to charmpp itself (and add a conflict statement depending on pmi variant) ?

I think this is saying that namd needs to load openmpi when charmpp uses openmpi - is that correct? Adding openmpi as a run dependency could be useful to ensure that the environment includes what you want, but IMO we should probably offer more flexibility in what packages are loaded into the environment (i.e. transitive link dependencies in some cases). An interim fix (if you are willing to wait for that) could be to explicitly load openmpi in the environment that uses namd with charmpp.

Side note: I merged #16003 but had to revert it because of other test interactions that have cropped up since that PR was created. There is a WIP PR at #16429. I anticipate finishing it by 5/5

s-sajid-ali · 2020-05-03T20:36:53Z

I think this is saying that namd needs to load openmpi when charmpp uses openmpi - is that correct?

The exact quote from the charm++ manual states :

After installing UCX, there are several supported process management interfaces (PMI) that
can be specified as options in order to build Charm++ with UCX. These include Simple PMI,
Slurm PMI, Slurm PMI 2 and PMIx (using OpenMPI). Currently, in order to use PMIx for
process management, it is required to have OpenMPI installed on the system. Additionally, 
in order to use the other supported process management interfaces, it is required to have 
a non-OpenMPI based MPI implementation installed on the system (e.g. Intel MPI, MVAPICH, 
MPICH, etc.).

Thus, it's a (runtime?) dependency of charm++ itself. I added mpi as a dependency for each pmi variant individually as well as a conflict for pmi!=pmix and +openmpi. As stated earlier, the pmi=pmix case is provided to those who have external installs of openmpi+pmix. This should load the correct mpi into the runtime environment for charm++ and any dependent packages as well.

I also added the git repo for namd as documented, but this fails on my workstation running centos-8 (though it works on my laptop running wsl2-ubuntu-18.04 ) with :

[sajid@xrmlite temp]$ git clone https://charm.cs.illinois.edu/gerrit/namd.git
Cloning into 'namd'...
fatal: unable to access 'https://charm.cs.illinois.edu/gerrit/namd.git/': error:1425F102:SSL 
routines:ssl_choose_client_version:unsupported protocol

Should I keep the git url for now and let the fetch options be fixed later or remove it ?

Thanks for cleaning up the mpi tests! Once #16429 is merged, the CI for this can be re-run.

This fixes some errors with setting up test configuration. These errors do not cause current Spack tests to fail but do create red herring issues elsewhere (see #15666). Fixing these errors leads to more errors in tests that depended on the original misconfigured state, so those are also addressed here. This is an update to #16003 which accounts for some unit tests with conflicting config/mutable_config fixtures. These conflicts were not exposed until the mutable_config fixture was fixed. Details are included below. The change which builds on #16003 is prefixed with "(new)". * For tests that use the real Spack package repository, the config needs to avoid using MPI providers that are not intended to be installed by Spack. Without this, it is possible that Spack tests which concretize the MPI virtual will end up trying to use an implementation that it shouldn't (e.g. one that is always provided externally). See #15666 for an example. * The mutable_config test fixture was not initializing the scope roots to the right directories (so the resulting config was empty). * The current_host fixture in the concretize.py tests was using the config fixture rather than mutable_config, and was polluting the config cache for other tests. * One test in concretize.py was clearing a nonexistent cache (PackagePrefs._packages_config_cache). This reference has been removed. * The test 'test_preferred_compilers' was was depending on cross test config pollution to succeed. The initial spec before concretization has been updated to updated to be explicit about the desired result. * (new) For tests that use install_mockery and mutable_config, replace install_mockery with a separate install_mockery_mutable_config fixture that is exactly the same as install_mockery but uses the mutable_config fixture to avoid conflicts.

…_wip_unstable

s-sajid-ali · 2020-05-11T19:34:04Z

@scheibelp : Any clue as to why the tests detect charmpp as an mpi provider even though this PR disables that ?

scheibelp · 2020-05-11T19:43:41Z

Any clue as to why the tests detect charmpp as an mpi provider even though this PR disables that ?

Let me try closing/reopening this - I'm trying to remember when the CI decides to rebase on develop and I think closing/reopening is one of the cases where it does.

s-sajid-ali · 2020-05-11T20:04:57Z

Looks like that didn't work, would merging develop into this branch help ?

scheibelp · 2020-05-11T20:28:25Z

would merging develop into this branch help ?

Yes that would work for sure - thanks!

…_wip_unstable

scheibelp · 2020-05-11T21:26:34Z

@alalazo just pointed out to me that there's a test that actually needs to be updated here: test_provider_lists is trying to assert that charmpp is an MPI provider (i.e. there was an additional problem to the one resolved by #16429), but this PR has removed it. Apologies to all for missing that.

@s-sajid-ali would you be able to update that unit test?

s-sajid-ali · 2020-05-11T22:40:49Z

@scheibelp : That fixed the issue! Could this PR get a final review now that the tests pass?

scheibelp · 2020-05-11T23:27:47Z

LGTM - thanks!

* charmc: make sure to link against pthread when using CkLoop Should help with issues such as the one seen here: spack/spack#15666 * dont link when SMP or Windows

s-sajid-ali and others added 15 commits February 24, 2020 00:17

WIP

fba9079

typo

1a45bf3

Merge branch 'develop' of https://github.com/spack/spack into charmpp…

c604012

…_ucx

fix docs

0d88634

cleanup

4b7f109

typo

b81f834

typo

140ddf9

flake8

41abc4a

fix openmpi+ucx

051ce76

fix backend to fabrics

ff4781c

prefixes!

80d56a4

unstablegit add --all

c4586ce

confused :(

54d02ee

minor

f79d455

try

0c896e1

s-sajid-ali changed the title ~~charmpp : fix prefix, introduce new property charmarch~~ [wip] charmpp : fix build prefix, introduce new property charmarch Mar 25, 2020

matthiasdiener reviewed Mar 25, 2020

View reviewed changes

var/spack/repos/builtin/packages/charmpp/package.py Outdated Show resolved Hide resolved

typo

b226c01

s-sajid-ali added 3 commits March 25, 2020 07:00

flake8

02514f1

incorporate spack#15667

5c430e7

flake8

83abb27

s-sajid-ali mentioned this pull request Mar 25, 2020

add new charmpp backend #15186

Closed

matthiasdiener reviewed Mar 25, 2020

View reviewed changes

var/spack/repos/builtin/packages/charmpp/package.py Show resolved Hide resolved

matthiasdiener reviewed Mar 25, 2020

View reviewed changes

Merge branch 'develop' into charmpp_wip_unstable

cefc479

scheibelp mentioned this pull request May 1, 2020

Revert "Testing: fix unintended interactions between tests (#16003)" #16420

Merged

s-sajid-ali added 4 commits May 3, 2020 14:45

fix inconsistencies

279059b

add mpi dependency as described in charm++ manual

81e4511

remove incorrect reference for pmi variant

675ee95

cleanup

c276c32

s-sajid-ali added 3 commits May 3, 2020 15:39

minor grammatical correction

3c2c7da

namd 2.14b1 release

aff1124

list charmpp dependency as per docs

edd2b8b

s-sajid-ali added 5 commits May 7, 2020 17:23

empty commit to rerun tests, post 16429 merge

22cdaf2

restart CI

22b8a52

Merge branch 'develop' of https://github.com/spack/spack into charmpp…

46ce669

…_wip_unstable

restart CI, now that the env-mod download page is back up

6534f59

restart CI

83a9467

scheibelp closed this May 11, 2020

scheibelp reopened this May 11, 2020

Merge branch 'develop' of https://github.com/spack/spack into charmpp…

793f9f8

…_wip_unstable

fix test_provider_lists

a300664

scheibelp merged commit b526a90 into spack:develop May 11, 2020

s-sajid-ali deleted the charmpp_wip_unstable branch July 1, 2020 22:59

Conversation

s-sajid-ali commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

matthiasdiener commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-sajid-ali commented Mar 25, 2020

Uh oh!

Uh oh!

matthiasdiener Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

s-sajid-ali Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

matthiasdiener Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

nitbhat Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

matthiasdiener commented Mar 25, 2020

Uh oh!

s-sajid-ali commented Mar 25, 2020

Uh oh!

matthiasdiener commented Mar 25, 2020

Uh oh!

scheibelp commented May 2, 2020

Uh oh!

s-sajid-ali commented May 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-sajid-ali commented May 11, 2020

Uh oh!

scheibelp commented May 11, 2020

Uh oh!

s-sajid-ali commented May 11, 2020

Uh oh!

scheibelp commented May 11, 2020

Uh oh!

scheibelp commented May 11, 2020

Uh oh!

s-sajid-ali commented May 11, 2020

Uh oh!

scheibelp commented May 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

s-sajid-ali commented Mar 25, 2020 •

edited

Loading

matthiasdiener commented Mar 25, 2020 •

edited

Loading

s-sajid-ali commented May 3, 2020 •

edited

Loading