netcdf: multiple improvements by aprokop · Pull Request #2377 · spack/spack

aprokop · 2016-11-21T22:16:05Z

Added 'dap' and 'cdmremote' variants
This is based on work in netcdf: Turn off DAP support by deafult. #2324 with the following motivation:
Turn off DAP support by deafult. DAP requires curl, which has issues
with circular dependencies. For 95% of NetCDF users that do not need
DAP, turning it off avoides this rats nest of problems.
Added 'parallel-netcdf' variant
To support work with parallel-netcdf
Added 'shared' and 'static' build separation

davydden · 2016-11-21T22:22:17Z

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

+
+        if '+dap' in spec or '+cdmremote' in spec:
+            # Make sure Netcdf links against Spack's curl, otherwise
+            # otherwise it may pick up system's curl, which can give link


two otherwise

alalazo

All the changes but the introduction of static look fine to me.

alalazo · 2016-11-22T03:34:36Z

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

+    variant('mpi',     default=True,  description='Enables MPI parallelism')
+    variant('hdf4',    default=False, description='Enable HDF4 support')
+    variant('shared',  default=True,  description='Enable shared library')
+    variant('static',  default=True,  description='Enable static library')


This is the only package that uses the static variant, and if I am not wrong we decided a while ago to use just shared. Anyhow: what would be the semantic of

spack install netcdf~shared~static

that is now allowed?

i agree with @alalazo , either build static always, or build static when ~shared.

@davydden @alalazo Is an overall Spack guidance for packages for shared/static builds documented somewhere?

@aprokop i don't think so. I create an issue for this #2380

@aprokop I thought it was, but sadly I cannot find it. What all the other packages do is to build both shared and static libraries when +shared and just static when ~shared.

See here:

https://groups.google.com/forum/#!topic/spack/gNAp1CmhxF0

citibeth

I'm concerned about depending on parallel-netcdf. What version of NetCDF are you building here? The latest version of NetCDF4 does not seem to have a configure flag named --enable-pnetcdf.

http://www.unidata.ucar.edu/software/netcdf/netcdf-4/newdocs/netcdf-install/Configure.html

AFAIK, Argonne's pnetcdf is now obsolete with NetCDF4.

adamjstewart · 2016-11-22T15:48:15Z

@citibeth If you download the latest version, 4.1.1.1, it does indeed have the --enable-pnetcdf flag available. I didn't know that it depended on parallel-netcdf though.

citibeth · 2016-11-22T16:06:56Z

@citibeth If you download the latest version, 4.1.1.1, it does indeed have the --enable-pnetcdf flag available. I didn't know that it depended on parallel-netcdf though.

Thanks, good detective work. Like you, without further evidence I'm still dubious that netcdf 4 would depend on parallel-netcdf.

aprokop · 2016-11-22T19:15:49Z

I'm concerned about depending on parallel-netcdf. What version of NetCDF are you building here? The latest version of NetCDF4 does not seem to have a configure flag named --enable-pnetcdf.

Oh, I did not even realize that. I'm building netcdf 4.3.3.1, and was told by a person of the code that it needs both --enable-pnetcdf and –enable-netcdf-4.

I will try to dig deeper to figure out when pnetcdf option is available.

aprokop · 2016-11-22T19:34:07Z

@adamjstewart If you download the latest version, 4.1.1.1

Did you mean 4.4.1.1?

According to this, the --enable-pnetcdf option appeared in 4.2.2.1.

Couple quick questions:

Is one supposed to hide a spec variant if it does not do anything for a particular spec version?
Is it possible to combine statement in when clause? Something like this:

    depends_on("parallel-netcdf", when='+parallel-netcdf' && '@4.2.1.1:')

Or do I have to do the if?

adamjstewart · 2016-11-22T19:39:58Z

Did you mean 4.4.1.1?

Yep, my bad

Is one supposed to hide a spec variant if it does not do anything for a particular spec version?

I don't think there is currently a way to make a variant unavailable for a certain version, although you can raise an error if someone tries to use it on a version where it is unavailable. See python+ucs4 for an example.

Is it possible to combine statement in when clause? Something like this:
   depends_on("parallel-netcdf", when='+parallel-netcdf' && '@4.2.1.1:')

Yep, it would look like this:

depends_on('parallel-netcdf', when='@4.2.1.1:+parallel-netcdf')

aprokop · 2016-11-22T19:41:50Z

Updated PR:

Got rid of static variant
The current spec always enables static
If ~shared, add -fPIC flag
This would have to be changed when Adding pic_flag property to compilers #2375 is merged
Update the requirement for parallel-netcdf variant
Fixed double otherwise misprint

I had to add -fPIC to CFLAGS instead of CPPFLAGS as the netcdf configure has this:

  CFLAGS      C compiler flags
  LDFLAGS     linker flags, e.g. -L<lib dir> if you have libraries in a
              nonstandard directory <lib dir>
  LIBS        libraries to pass to the linker, e.g. -l<library>
  CPPFLAGS    (Objective) C/C++ preprocessor flags, e.g. -I<include dir> if
              you have headers in a nonstandard directory <include dir>

adamjstewart · 2016-11-22T19:45:34Z

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

-    depends_on("[email protected]:")
+    depends_on("[email protected]:", when='+dap')
+    depends_on("[email protected]:", when='+cdmremote')
+    if spec.satisfies('@4.2.2.1:'):


spec is not defined in this context. You'll need to try my suggestion of putting the version in when.

@adamjstewart Well spotted!

- Added 'dap' and 'cdmremote' variants This is based on work in spack#2324 with the following motivation: Turn off DAP support by deafult. DAP requires curl, which has issues with circular dependencies. For 95% of NetCDF users that do not need DAP, turning it off avoides this rats nest of problems. - Added 'parallel-netcdf' variant To support work with parallel-netcdf - Added 'shared' and 'static' build separation

aprokop · 2016-11-22T19:46:32Z

Updated PR with @adamjstewart comments on when.

adamjstewart · 2016-11-22T19:52:08Z

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

+        # PnetCDF support
+        if '+parallel-netcdf' in spec:
+            config_args.append('--enable-pnetcdf')
+            config_args.append('CC=%s' % spec['mpi'].mpicc)


What happens when someone tries to build netcdf+parallel-netcdf~mpi?

I guess I don't know. The simplest way would be to prohibit parallel-netcdf without mpi, but again I don't know if it makes sense.

Also, out of curiosity, what exactly is the difference between netcdf+mpi and netcdf+parallel-netcdf? I've never used either, but my users regularly use serial netcdf and parallel-netcdf.

The simplest way would be to prohibit parallel-netcdf without mpi

i would say thrown an error if someone tries to do it.

citibeth · 2016-11-22T20:35:13Z

Something in me is not comfortable with a variant having a dash in it.
Could we rename +parallel-netcdf --> +pnetcdf? Would also rename the
package similarly. This would not be inappropriate, since the library
calls itself both:

https://trac.mcs.anl.gov/projects/parallel-netcdf

The difference is... pnetcdf is a reimplementation of the NetCDF-3 file
format for parallel. It is from Argonne National Lab and completely
different and separate from the NetCDF effort at Unidata.

NetCDF4 is no longer a format itself; it is a front-end on top of an HDF5
backend --- and a NetCDF3 backend for compatibility. The recent change
pointed to above allows one to use pnetcdf as a backend as well.

If building a new project, I would use NetCDF4+HDF5, since HDF5 also offers
parallel capabilities, and its file format has a lot of useful features not
found in the NetCDF3 format supported by pnetcdf.

On Tue, Nov 22, 2016 at 3:03 PM, Adam J. Stewart [email protected]
wrote:

@adamjstewart commented on this pull request.

In var/spack/repos/builtin/packages/netcdf/package.py:

@@ -131,10 +151,18 @@ def install(self, spec, prefix):
LDFLAGS.append("-L%s/lib" % spec['szip'].prefix)
LIBS.append("-l%s" % "sz")
   # PnetCDF support
   if '+parallel-netcdf' in spec:
       config_args.append('--enable-pnetcdf')
       config_args.append('CC=%s' % spec['mpi'].mpicc)
Also, out of curiosity, what exactly is the difference between netcdf+mpi
and netcdf+parallel-netcdf? I've never used either, but my users
regularly use serial netcdf and parallel-netcdf.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2377, or mute the thread
https://github.com/notifications/unsubscribe-auth/AB1cd0Ur9MQLSzwqlISIsMqL-hx7gfRTks5rA0qmgaJpZM4K4w-g
.

citibeth · 2016-11-22T20:39:13Z

Does the --enable-pnetcdf option require MPI directly? Meaning... does
NetCDF make MPI calls directly in that case? Or does it just use MPI
through pnetcdf, in which case we don't have to do anything?

On Tue, Nov 22, 2016 at 3:28 PM, Denis Davydov [email protected]
wrote:

@davydden commented on this pull request.

In var/spack/repos/builtin/packages/netcdf/package.py:

@@ -131,10 +151,18 @@ def install(self, spec, prefix):
LDFLAGS.append("-L%s/lib" % spec['szip'].prefix)
LIBS.append("-l%s" % "sz")
   # PnetCDF support
   if '+parallel-netcdf' in spec:
       config_args.append('--enable-pnetcdf')
       config_args.append('CC=%s' % spec['mpi'].mpicc)
The simplest way would be to prohibit parallel-netcdf without mpi

i would say thrown an error if someone tries to do it.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2377, or mute the thread
https://github.com/notifications/unsubscribe-auth/AB1cd_QZRMeWULj5zT7b953eLFpSC9xHks5rA1CLgaJpZM4K4w-g
.

adamjstewart · 2016-11-22T20:45:31Z

I agree with renaming the variant to pnetcdf, since that's what the actual configure flag looks like. As for the name of the parallel-netcdf package, I'm indifferent.

aprokop · 2016-11-22T22:21:25Z

Does the --enable-pnetcdf option require MPI directly? Meaning... does
NetCDF make MPI calls directly in that case? Or does it just use MPI
through pnetcdf, in which case we don't have to do anything?

I'm actually not sure any more. Looking at the old spec, +mpi adds --enable-parallel4 config option, but for instance 4.3.3.1 does not have it (I think it only appeared in 4.4).

Few interesting bits of information from here:

The dependencies of netcdf are only these:
- HDF5 1.8.9 or later (for netCDF-4 support)
- zlib 1.2.5 or later (for netCDF-4 compression)
- curl 7.18.0 or later (for DAP remote access client support)
But in the Building with parallel support section it says to use mpicc to compile netcdf when used with pnetcdf or with parallel HDF5

So, I think it does need MPI when compiling with +pnetcdf or with ^hdf5+mpi. I'm less sure now of whether I need the

config_args.append('CC=%s' % spec['mpi'].mpicc)

line as I only needed that for static builds, and I have not checked for shared.

In summary, a) I will rename the variant to pnetcdf; b) I'll make sure +pnetcdf requires +mpi and leave it at that; and c) I will also check whether it works without explicit mpicc.

One of the remaining questions I have: it seems that pnetcdf and hdf5 alternatives to each other?
If true, then logic of mpi, hdf5+mpi and pnetcdf becomes even more convoluted. Personally, I'm not sure which combinations would be OK.

Regarding the parallel-netcdf package renaming, I have no opinion.

citibeth · 2016-11-22T22:45:32Z

Consider using the netcdf cmake build instead of autotools. Cmake knows
how to set pic flags for different compilers.
On Nov 22, 2016 2:42 PM, "Andrey Prokopenko" [email protected]
wrote:

Updated PR:

Got rid of static variant
The current spec always enables static

If ~shared, add -fPIC flag
This would have to be changed when Adding pic_flag property to compilers #2375
Adding pic_flag property to compilers #2375 is merged

Update the requirement for parallel-netcdf variant

Fixed double otherwise misprint

I had to add -fPIC to CFLAGS instead of CPPFLAGS as the netcdf configure
has this:

CFLAGS C compiler flags
LDFLAGS linker flags, e.g. -L if you have libraries in a
nonstandard directory
LIBS libraries to pass to the linker, e.g. -l
CPPFLAGS (Objective) C/C++ preprocessor flags, e.g. -I if
you have headers in a nonstandard directory

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2377 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AB1cd-RadPyb9GbwqvwakEmVIPja6WCQks5rA0WAgaJpZM4K4w-g
.

aprokop · 2016-11-22T22:47:15Z

Consider using the netcdf cmake build instead of autotools. Cmake knows
how to set pic flags for different compilers.

Would not that require to completely redo the whole spec?

adamjstewart · 2016-11-22T22:48:38Z

We've discussed CMake vs. Autotools for the NetCDF package before: #400 (comment)

We decided to stick with Autotools because it can build older versions of NetCDF. I also don't see any problems with the current Autotools build.

davydden · 2016-11-22T23:00:24Z

I would keep 'parallel-netcdf' as package name as it is self-explanatory, whereas pnetcdf to me sounds close to python namings (i.e. py-netcdf).

tgamblin · 2016-11-24T20:25:11Z

@aprokop @citibeth: how about parallel for the variant name? We've used this on some other packages. I'm sometimes not a fan of it because parallel can mean MPI, OpenMP, pthreads, or whatever, but there is really only one type of parallel NetCDF and I think the concept of parallel I/O, at least in HPC, is fairly well understood. Decent compromise?

citibeth · 2016-11-24T20:44:02Z

I would rather just keep `parallel-netcdf`

…

On Thu, Nov 24, 2016 at 3:25 PM, Todd Gamblin ***@***.***> wrote: @aprokop <https://github.com/aprokop> @citibeth <https://github.com/citibeth>: how about parallel for the variant name? We've used this on some other packages. I'm sometimes not a fan of it because parallel can mean MPI, OpenMP, pthreads, or whatever, but there is really only one type of parallel NetCDF and I think the concept of parallel I/O, at least in HPC, is fairly well understood. Decent compromise? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2377 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd6Yi_vcOIK6yy5KEfDG85TU0HEh7ks5rBfKogaJpZM4K4w-g> .

tgamblin · 2016-11-24T21:52:20Z

Going with parallel-netcdf as I'm leaning more towards that now that I see it's the same name as the dependency (so I think it is clearer). If people feel strongly please submit a new PR and we can debate the name.

- Added 'dap' and 'cdmremote' variants This is based on work in spack#2324 with the following motivation: Turn off DAP support by deafult. DAP requires curl, which has issues with circular dependencies. For 95% of NetCDF users that do not need DAP, turning it off avoides this rats nest of problems. - Added 'parallel-netcdf' variant To support work with parallel-netcdf - Added 'shared' and 'static' build separation

davydden approved these changes Nov 21, 2016

View reviewed changes

adamjstewart approved these changes Nov 21, 2016

View reviewed changes

alalazo requested changes Nov 22, 2016

View reviewed changes

alalazo mentioned this pull request Nov 22, 2016

netcdf: Turn off DAP support by deafult. #2324

Closed

citibeth reviewed Nov 22, 2016

View reviewed changes

aprokop force-pushed the netcdf_improve branch from 0b1d92d to 94a5447 Compare November 22, 2016 19:41

alalazo approved these changes Nov 22, 2016

View reviewed changes

adamjstewart reviewed Nov 22, 2016

View reviewed changes

aprokop force-pushed the netcdf_improve branch from 94a5447 to d658d39 Compare November 22, 2016 19:45

adamjstewart reviewed Nov 22, 2016

View reviewed changes

davydden approved these changes Nov 22, 2016

View reviewed changes

tgamblin merged commit 8f0b91e into spack:develop Nov 24, 2016

aprokop deleted the netcdf_improve branch December 7, 2016 17:28

skosukhin mentioned this pull request Oct 19, 2017

Update for 'netcdf'. #5819

Merged

Conversation

aprokop commented Nov 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

citibeth left a comment

Choose a reason for hiding this comment

Uh oh!

adamjstewart commented Nov 22, 2016

Uh oh!

citibeth commented Nov 22, 2016

Uh oh!

aprokop commented Nov 22, 2016

Uh oh!

aprokop commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented Nov 22, 2016

Uh oh!

aprokop commented Nov 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aprokop commented Nov 22, 2016

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!

citibeth commented Nov 22, 2016

@adamjstewart commented on this pull request.

Uh oh!

citibeth commented Nov 22, 2016

@davydden commented on this pull request.

Uh oh!

adamjstewart commented Nov 22, 2016

Uh oh!

aprokop commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

citibeth commented Nov 22, 2016

Uh oh!

aprokop commented Nov 22, 2016

Uh oh!

adamjstewart commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davydden commented Nov 22, 2016

Uh oh!

tgamblin commented Nov 24, 2016

Uh oh!

citibeth commented Nov 24, 2016 via email

Uh oh!

tgamblin commented Nov 24, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

aprokop commented Nov 22, 2016 •

edited

Loading

aprokop commented Nov 22, 2016 •

edited

Loading

adamjstewart commented Nov 22, 2016 •

edited

Loading