Update for 'hdf5'. by skosukhin · Pull Request #5790 · spack/spack

skosukhin · 2017-10-17T09:46:31Z

An update for HDF5:

some refactoring;
allows for thread safety and high-level interface at the same time ('hl' becomes a variant);
returns 'unsupported' variant (see Removing unsupported variant from HDF5. #520) to allow unsupported combinations only when it's specified explicitly;
additional conflict statement for mpi and cxx.

I would also make ~mpi by default because every time we want to install something that needs just NetCDF we have to write: install A+netcdf ^netcdf~mpi ^hdf5~mpi to avoid compilation of MPI library. But I don't know which case is more often.

There will be a discussion probably.

davydden · 2017-10-17T10:10:09Z

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


-    variant('cxx', default=True, description='Enable C++ support')
-    variant('fortran', default=True, description='Enable Fortran support')
+    variant('unsupported', default=False,


I am not in favor of this. This variant does NOT change package behavior anyhow, as discussed in the issue u linked

To clarify: If I understand correctly, say with default settings, hdf5~unsupported and hdf5+unsupported are exactly the same builds. No functional difference. Thus it shall not be a variant and was removed.

I don't insist on this but why are there 'conflicts' statements in the current script and how can I tell spack to ignore them? For example, if I want to enable the cxx interface and have the thread safety. The developers of hdf5 allow for that. Of course, I have to take the responsibility explicitly (--enable-unsupported) in that case. The 'unsupported' variant is for the same reason: to allow package developers (and users) to specify that they need/want an unsupported combination of features and they know what they are doing. Otherwise, they get a notification from the 'conflicts' statement.

Then just remove the conflicts statements. Probably they should have been removed earlier together with unsupported.

One way or another, variants are meant to change the package, which is not the case here. Similarly “run-tests” is not a variant, instead we have —run-tests option of install command

@skosukhin For what is worth, my interpretation at the time I did this was:

+cxx+mpi is claimed to be unsupported -> there's no promise on its behavior

+cxx+threadsafe is claimed to be not compatible -> it's known to be broken

+cxx+mpi is claimed to be unsupported -> there's no promise on its behavior

that we can probably allow, but maybe print a warning to the user during installation.

+cxx+threadsafe is claimed to be not compatible -> it's known to be broken

then this shall stay as a conflict.

+cxx+threadsafe is claimed to be not compatible -> it's known to be broken

@alalazo , does this mean that the 'threadsafe' feature doesn't work even via low-level interface (when +cxx) or only via cxx interface?

@skosukhin I would expect only through the cxx interface (and I hope the low-level is compiled the same regardless of cxx). Don't take my word for it, though - never peeked the code inside hdf5, and the docs are not 100% clear.

davydden · 2017-10-17T10:10:45Z

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

+    variant('unsupported', default=False,
+            description='Allow unsupported combinations of configure options')
+    variant('hl', default=False, description='Enable the high-level library')
+    variant('cxx', default=False, description='Enable C++ support')


~~Why change to false?~~

given the reply of @alalazo , let's add an in-code comment above along the lines

# +cxx+mpi is not explicitly supported, one can build HDF5 that way but there's no promise on its behavior

As far as I understand, we have to introduce unnecessary constraints (~cxx) in the depends_on statement to enable thread safety (+threadsafe) even if we don't really care if the cxx interface is enabled or not. It might cause a conflict if there are two packages in a dependency tree one requiring +cxx, and another one requiring thread safety.

we have to introduce unnecessary constraints

If I understand you right, conflicts('+threadsafe', when='+cxx') is perfectly valid one and shall remain there assuming that

+cxx+threadsafe is claimed to be not compatible -> it's known to be broken

is indeed the case. We only remove it if proven otherwise. So i would not call it "unnecessary".

if there are two packages in a dependency tree one requiring +cxx, and another one requiring thread safety.

you can't do anything about that. They simply require incompatible variants. This shall be resolved upstream (hdf5).

They simply require incompatible variants.

One of the packages uses low-level C-API in multiple threads, another one uses CXX interface in a single thread. As far as I understand, both can get what they need from the same installation of the library if it's configured with +cxx+threadsafe.

both can get what they need from the same installation of the library if it's configured with +cxx+threadsafe.

then, feel free to add this comment and comment out (instead of removing) the conflict statement so that no one tries to re-add it later.

davydden · 2017-10-17T10:10:54Z

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

+            description='Allow unsupported combinations of configure options')
+    variant('hl', default=False, description='Enable the high-level library')
+    variant('cxx', default=False, description='Enable C++ support')
+    variant('fortran', default=False, description='Enable Fortran support')


davydden · 2017-10-17T10:13:56Z

Since Spack is Supercomputer package manager, we try to keep all MPI on by default

skosukhin · 2017-10-17T13:33:52Z

Since Spack is Supercomputer package manager, we try to keep all MPI on by default

In Spack we can easily tell what we need: packages that need hdf5+mpi have depends_on('hdf5+mpi') statement anyway. The problem is that Spack does not allow us to specify what we don't need: with depends_on('hdf5~mpi') we actually say that we need hdf5 without mpi support. So, how do we tell that we don't care=don't need? And if we don't need smth than why do we want to compile it? I understand when applications are compiled with extra dependencies because there might be a well-established set of features that users expect. But why do we need to have extra dependencies by default for libraries?

junghans · 2017-10-17T15:46:40Z

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

-    variant('cxx', default=True, description='Enable C++ support')
-    variant('fortran', default=True, description='Enable Fortran support')
+    variant('hl', default=False, description='Enable the high-level library')
+    variant('cxx', default=False, description='Enable C++ support')


Changing the old default?

davydden · 2017-10-17T19:11:01Z

So, how do we tell that we don't care=don't need?

depends_on('hdf5') means exactly this -- we don't care whether it's +mpi or ~mpi.

skosukhin · 2017-10-19T11:35:22Z

The problem is that if we build with MPI support by default and we don't need MPI and don't want to compile it, we have to disable 'mpi' variant for all the dependencies: A~mpi ^B~mpi ^C~mpi. Otherwise, if we don't enable MPI support by default and we need it, we specify it only once: A+mpi (the rest is taken care of by depends_on() statements).

davydden · 2017-10-19T11:36:54Z

I disagree in the assessment that it's a problem with rationale above #5790 (comment)

davydden · 2017-10-19T11:44:20Z

I also recall that there were some issues with concretizer if you don't set +mpi by default. @adamjstewart might know more on this topic.

skosukhin · 2017-10-19T12:07:51Z

I disagree in the assessment that it's a problem with rationale above

So much work done to make Spack user-friendly and it's not a problem? So much work done to make Spack work even on MacOS (and other desktop environments) and the rationale remains? So many pieces of software that depend on libraries but don't require their MPI features but we still have to compile MPI libraries by default? Well, fine.

davydden · 2017-10-19T12:12:22Z

So much work done to make Spack user-friendly and it's not a problem?

the point is that you can still get to what you want, just use ~/.spack/packages.yaml to get all the preferences if you don't like command line arguments. I am just saying that Spack (from my understanding) is targeted at HPC. So in 99% of cases you will have MPI around, and thereby there is no harm of building with MPI by default. Heck, I even have MPI on my macbook to develop and debug things. But as always, YMMV.

adamjstewart · 2017-10-19T16:11:28Z

I also recall that there were some issues with concretizer if you don't set +mpi by default. @adamjstewart might know more on this topic.

The issue arises whenever you have packages A, B, C that all share the same variant X and A+X depends on B+X depends on C+X. If A, B, C don't all have the same default, the concretizer gets confused. Basically, it chooses the default for C regardless of what A needs. This will hopefully be fixed in the future, but for now, the safest thing to do is make sure that X always has the same default.

X only matters when it is shared by many packages. +mpi is an example of this. Since there are a lot of packages with an +mpi variant that depend on hdf5+mpi, hdf becomes our C in the example above, and we need to make sure that all packages with an mpi variant have the same default.

Spack is a Supercomputing PACKage manager after all, so we (I) decided to make the default +mpi instead of ~mpi. If you don't want MPI support, you can override this in packages.yaml.

See #2590 for the PR where this policy was introduced. It (hopefully) has a good summary of what the problem is and how far-reaching it is.

Refactoring for 'hdf5'.

1c449c8

davydden suggested changes Oct 17, 2017

View reviewed changes

davydden requested a review from alalazo October 17, 2017 10:34

Remove 'unsupported' variant.

fa42945

davydden approved these changes Oct 17, 2017

View reviewed changes

junghans approved these changes Oct 17, 2017

View reviewed changes

tgamblin approved these changes Oct 24, 2017

View reviewed changes

tgamblin merged commit 161dca6 into spack:develop Oct 24, 2017

skosukhin deleted the hdf5 branch November 7, 2017 14:13

skosukhin mentioned this pull request May 22, 2020

NetCDF: fix constraints 2 #16719

Merged

Conversation

skosukhin commented Oct 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davydden Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davydden Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davydden Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davydden Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davydden commented Oct 17, 2017

Uh oh!

skosukhin commented Oct 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davydden commented Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skosukhin commented Oct 19, 2017

Uh oh!

davydden commented Oct 19, 2017

Uh oh!

davydden commented Oct 19, 2017

Uh oh!

skosukhin commented Oct 19, 2017

Uh oh!

davydden commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented Oct 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

davydden Oct 17, 2017 •

edited

Loading

davydden Oct 17, 2017 •

edited

Loading

davydden Oct 17, 2017 •

edited

Loading

davydden Oct 17, 2017 •

edited

Loading

davydden commented Oct 17, 2017 •

edited

Loading

davydden commented Oct 19, 2017 •

edited

Loading