add package for conduit #2670

cyrush · 2016-12-22T22:01:52Z

adds spack package for conduit (http://software.llnl.gov/conduit)

adamjstewart · 2016-12-22T22:47:40Z

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

+
+import socket
+import os
+from os.path import join as pjoin


You can just use join_path. It's already imported from from spack import *.

sounds good, will do.

adamjstewart · 2016-12-22T22:49:37Z

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

+    variant("shared", default=True, description="Build Conduit as shared libs")
+
+    variant("cmake", default=True,
+            description="Build CMake (if off, attempt to use cmake from PATH)")


You should probably just always build with Spack's CMake. No sense in adding a variant that doesn't really change how the package is built.

Building cmake on bgq is an issue, that is the only reason this exists.

Could you add CMake to your packages.yaml as an external package? Are you able to build any of the other packages in Spack that depend on CMake?

I agree with @adamjstewart, packages should not build their own dependencies. Let's figure out how to build CMake on bgq; and until then, as @adamjstewart suggestes, use one that's already installed.

So... the cmake variant probably needs to go.

@mathstuf You work at Kitware, right? Are you aware of this problem with building CMake on bgq?

No one has filed an issue, so no.

@cyrush: the cmake on BG/Q issue will be gone one #2548 is in -- that will prefer to build build-only deps with the "default" front-end compiler.

@tgamblin I think before many of the bells and whistles were added to the cmake package we used to be able to build on cmake on BGQ. Constraining the deps to build statically and only with the front end compiler were both challenges, which is why we gave up and have started using the system install (only on BGQ)

adamjstewart · 2016-12-22T22:52:22Z

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

+    #######################
+    # Python
+    #######################
+    depends_on("python", when="+python")


depends_on or extends? The only real difference is that if you extend Python, you can activate conduit and symlink any site-packages directories to the Python installation.

Is there there a good example you have in mind?

Not sure if extends will work with my compiled python modules. Conduit has more options to control where the python modules are installed, but I am not using those here.

See here for an example of a package that produces a compiled Python module:
https://github.com/citibeth/spack/blob/efischer/develop/var/spack/repos/builtin/packages/icebin/package.py

Also, what version(s) of Python does your compiled module work with? 2? or 3? or both? Assuming it's 2, you should probably have the following:

extends('python') depends_on('python@2:2.7')

conduit works with python 2 and 3 -- I'll try extends

adamjstewart · 2016-12-22T22:52:40Z

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

+    # Python
+    #######################
+    depends_on("python", when="+python")
+    depends_on("py-numpy~blas~lapack", when="+python")


Can it not be built with py-numpy+blas+lapack?

correct, numpy with the blas variant fails to build on my osx machine

Hmm, that seems like a numpy problem, not a conduit problem. You may want to set that in your packages.yaml and allow users to build conduit with whatever they want.

I agree its a numpy issue -- but whats a good strategy here? This package file is a swizzle on what we use for automated builds for a team of folks. Settings things up to be as out of the box as possible is important to us.
Over time I would gladly remove these constraints as more testing protects us.

Well, I don't use conduit, so it obviously doesn't really bother me. But the way that things should work is that you can build it with whatever dependencies work. If something like py-numpy+blas+lapack doesn't build for you, you should add the following to your packages.yaml:

packages: py-numpy: variants: ~blas~lapack

Then, it should work for you. But like you said, if you are trying to get it working out of the box and deploying it to other users who are on bgq, this isn't a valid solution. @tgamblin Can we add a bgq defaults packages.yaml like we did for macOS?

@adamjstewart is right; packages need to be OS-agnostic to as great an extent as possible. package.yaml is for machine-specific configuration. This package should just read:

depends_on("py-numpy", when="+python")

Again... if you have trouble building py-numpy with BLAS on bgq, then you'll have a whole lot of packages to worry about, that also depend on py-numpy. Setting it once in packages.yaml is a better solution.

Note: this is actually a problem on osx, not BGQ.

for the record, numpy builds fine for me on OS-X Sierra with the current develop (3f1cdbf). If it's still a problem, please file a separate bug report with detailed info. Whereas here py-numpy shall not be restricted to ~blas~lapack as it is NOT a requirement from the pakage, but a workaround to build py-numpy.

p.s. also check out this #2365

Sorry, assumed it was a bgq problem, not a macOS problem. @davydden is right. Probably best to file a separate issue and modify your packages.yaml for now. Someday numpy and cmake will be stable on all OSes.

@cyrush: I don't mind if you need these defaults for now but this should be a todo for us to make sure all the numpy variants (or at least the defaults) actually build properly on macOS. 1.0 in February will have package testing, so I think at that point at least you can remove the constraint.

adamjstewart · 2016-12-22T22:55:38Z

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

+    depends_on("hdf5~shared~cxx~mpi~fortran", when="~shared")
+
+    depends_on("silo+shared~fortran", when="+shared")
+    depends_on("silo~shared~fortran", when="~shared")


These seem overly restricting. Can they really not be built with hdf5+cxx+mpi or silo+fortran? Also, try running spack spec conduit ^hdf5@1.8.12. You'll find that it complains that conduit doesn't depend on hdf5. The problem is that Spack's concretizer doesn't evaluate default dependencies until after it resolves the things you specify. You'll have to add:

depends_on('hdf5') depends_on('silo')

These are all due to various build issues, many on bgq ...

If I add depends_on('hdf5') will that clear the variants above?

I understand there are limits to how variants work, but I am willing to manually forward things and be over-narrow to get a working build.

If you're having problems building hdf5 on bgq, you should add the default variants that work for you to your packages.yaml. We can try working out the hdf5 issues separately.

I think I will eventually be able to do this for our team (we have an automated process that wraps spack) -- but there is something to be said about having the spack recipe stand on its own for a wide set of folks. I am in favor of more restrictive as a way to improve the likelihood of success.

One important clarification:

These selections are not only due to build issues.

For instance, I explicitly don't want MPI dependencies for HDF5 in Conduit. But I certainly do not want to disable MPI support in HDF5 for someone else on BGQ that would want to use it. B/c of this I am not sure a spack-wide packages.yaml helps.

I agree with @adamjstewart, this should read:

depends_on('hdf5', when='+hdf5') depends_on('silo', when='+silo')

If package A depends on package B (A->B), then A+shared will link just fine with B+static. So there is no need to forward the shared variant. We don't do it elsewhere. Users who need to build static will need to set the +static (or ~shared; we're not yet quite sure which) on every package that can take it. We're still considering good ways to do that; see Define conventions for shared / static builds #2380 Variant Forwarding Proposal #2594 Re-work shared vs. static builds (Universal Variants) #2492 Global Variants #2165 Allow setting default variants #2644

packages.yaml can be set at many levels: per-Spack, per-user, etc. See: https://spack.readthedocs.io/en/latest/configuration.html It's more than just Spack-wide.

These selections are not only due to build issues.

Then they should definitely go in packages.yaml.

I can pull the +shared if that is a standard and always on

These are all due to various build issues, many on bgq ...

I agree with others and I am not in favour of over-constraining (~cxx~fortran) things only because something fails to build on some platforms. If there are issues, they need to be addressed separately.

I explicitly don't want MPI dependencies for HDF5 in Conduit

that's ok as long as it's a requirement of Conduit, not a build workaround.

So it sounds like we've agreed upon:

depends_on('hdf5~mpi', when='+hdf5') depends_on('silo', when='+silo')

No... As long as Conduit works with hdf5+mpi, then that should be allowed. It should just depends_on('hdf5', when='+hdf5'). If users don't want MPI, that's again a packages.yaml issue.

adamjstewart · 2016-12-22T23:00:06Z

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

+            cmake_args.extend(["-C", host_cfg_fname, "../src"])
+            cmake(*cmake_args)
+            make()
+            make("install")


You should consider extending CMakePackage. It will save you from a lot of duplicated code. Let me know if you need any pointers on how to do that.

how much would it save me? Also worried about rpath settings for the static case.

I thought our CMake support automatically did the RPATH stuff. @alalazo?

It does, but I had problems when building static-ly

#2658

Again, bgq ...

Soo glad I don't have to deal with bgq 😄

missing out on all of the fun.
(actually I can't type that with a straight face, you are a lucky individual ...)

how much would it save me?

It's not about saving the package author; but about making things standard in a way that's more maintainable down the line.

It looks like this is a good solution to #2658? If that's the case, then we should put it in CMakePackage, activated on the condition it's running on bgq. It should not go in this PR: because it could mess up people NOT running on bgq, and because it won't fix the problem for bgq users of all the other CMake-based packages in our repo.

BTW... are shared libraries possible?

cyrush · 2016-12-22T23:26:06Z

Thanks for the comments @adamjstewart . I updated to use develop spack today (I had been working on a very old version) and updated my old package file. So I was oblivious to some of the new features (CMakePackage for example)

citibeth · 2016-12-23T02:52:13Z

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

+    variant("shared", default=True, description="Build Conduit as shared libs")
+
+    variant("cmake", default=True,
+            description="Build CMake (if off, attempt to use cmake from PATH)")


I agree with @adamjstewart, packages should not build their own dependencies. Let's figure out how to build CMake on bgq; and until then, as @adamjstewart suggestes, use one that's already installed.

So... the cmake variant probably needs to go.

citibeth · 2016-12-23T02:53:29Z

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

+    #######################
+    # CMake
+    #######################
+    depends_on("cmake@3.3.1", when="+cmake")


This should always depends_on('cmake@3.3.1'). Spack already has ways to use a CMake from $PATH if that's what you want.

Will this work with later versions of CMake? Mabye depends_on('cmake@3.3.1:')?

Need a specific version of CMake, later ones have bugs, earlier ones lack features

@cyrush: is BLT not compatible across different CMake versions? I realize that is probably CMake's fault but what bugs are in the way?

conduit doesn't use BLT, but the problem is similar. Newer versions of cmake, when looking for python find both the so and the symlink to the main python lib, which breaks some of the logic deep in the CMake python macros. Happens on linux, and in particular on chaos -- but not sure how wide spread. We are going to work on a fix for cmake but haven't had the time to yet.

Also, we haven't really tested with newer versions of CMake, so I wouldn't assume they work. In my world specific and tested leads to better outcomes.

@cyrush: oh so this is a bug with CMake's generally crappy Python detection? I think a lot of projects get around that with their own FindPython. Ugh.

citibeth · 2016-12-23T02:55:49Z

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

+    #######################
+    # Python
+    #######################
+    depends_on("python", when="+python")


See here for an example of a package that produces a compiled Python module:
https://github.com/citibeth/spack/blob/efischer/develop/var/spack/repos/builtin/packages/icebin/package.py

Also, what version(s) of Python does your compiled module work with? 2? or 3? or both? Assuming it's 2, you should probably have the following:

extends('python') depends_on('python@2:2.7')

citibeth · 2016-12-23T02:56:53Z

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

+
+    version('0.2.0', 'd595573dedf55514c11d7391092fd760')
+
+    version('github-master', git='https://github.com/LLNL/conduit.git')


I would just call this version master

citibeth · 2016-12-23T03:01:12Z

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

+    # Python
+    #######################
+    depends_on("python", when="+python")
+    depends_on("py-numpy~blas~lapack", when="+python")


@adamjstewart is right; packages need to be OS-agnostic to as great an extent as possible. package.yaml is for machine-specific configuration. This package should just read:

depends_on("py-numpy", when="+python")

Again... if you have trouble building py-numpy with BLAS on bgq, then you'll have a whole lot of packages to worry about, that also depend on py-numpy. Setting it once in packages.yaml is a better solution.

citibeth · 2016-12-23T03:05:47Z

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

+            description="Build CMake (if off, attempt to use cmake from PATH)")
+
+    # variants for python support
+    variant("python", default=True, description="Build Conduit Python support")


This should almost certainly be default=False. Python+numpy+etc. can be a real bear. No need to bring it in unless people know they want it.

citibeth · 2016-12-23T03:15:46Z

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

+    depends_on("hdf5~shared~cxx~mpi~fortran", when="~shared")
+
+    depends_on("silo+shared~fortran", when="+shared")
+    depends_on("silo~shared~fortran", when="~shared")


I agree with @adamjstewart, this should read:

depends_on('hdf5', when='+hdf5') depends_on('silo', when='+silo')

If package A depends on package B (A->B), then A+shared will link just fine with B+static. So there is no need to forward the shared variant. We don't do it elsewhere. Users who need to build static will need to set the +static (or ~shared; we're not yet quite sure which) on every package that can take it. We're still considering good ways to do that; see Define conventions for shared / static builds #2380 Variant Forwarding Proposal #2594 Re-work shared vs. static builds (Universal Variants) #2492 Global Variants #2165 Allow setting default variants #2644

packages.yaml can be set at many levels: per-Spack, per-user, etc. See: https://spack.readthedocs.io/en/latest/configuration.html It's more than just Spack-wide.

These selections are not only due to build issues.

Then they should definitely go in packages.yaml.

citibeth · 2016-12-23T03:18:59Z

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

+            cmake_args.extend(["-C", host_cfg_fname, "../src"])
+            cmake(*cmake_args)
+            make()
+            make("install")


how much would it save me?

It's not about saving the package author; but about making things standard in a way that's more maintainable down the line.

It looks like this is a good solution to #2658? If that's the case, then we should put it in CMakePackage, activated on the condition it's running on bgq. It should not go in this PR: because it could mess up people NOT running on bgq, and because it won't fix the problem for bgq users of all the other CMake-based packages in our repo.

BTW... are shared libraries possible?

citibeth · 2016-12-23T03:24:49Z

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

+        cfg.write("##################################\n")
+        cfg.write("# %s-%s\n" % (sys_type, spec.compiler))
+        cfg.write("##################################\n\n")
+


This whole section could be made a LOT more clear using str.format(*args, **kwargs) available in Python 2.6. For example:

cfg.write("""####################### # spack generated host-config ####################### # {sys_type}-{compiler} ################## """.format(sys_type=sys_type, compiler=spec.compiler)

The cmake_cache_entry() helper function can also be eliminated this way.

citibeth · 2016-12-23T03:31:44Z

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

+        Build and install Conduit.
+        """
+        with working_dir('spack-build', create=True):
+            host_cfg_fname = self.create_host_config(spec, prefix)


A CMake-based package that relies on a manually-built host configuration file is highly unusual, to say the least. Can Conduit be configured in the usual way by running cmake? This is especially effective when using Spack, since Spack already prevents the "wrong" version of libraries from polluting the build by keeping the env clean and having just one package per install directory. If you can get rid of the manually-built host config file and let CMake write that file for you, then MOST of the code in this package goes away.

citibeth · 2016-12-23T03:34:41Z

My general comments...

Please think about users outside of LLNL. There's nothing wrong with using a package that's highly tailored to the LLNL environment, and you can keep that in your own Spack fork. But packages in the public Spack repo need to work in general environments.
This package would "go down" better if it used CMake in a standard way. That would also make it easier for non-Spack users to install your software.

cyrush · 2016-12-23T15:05:21Z

@citibeth Thanks for the comments, I am worried about the suggestions to rely on package.yaml to tweak things for different sets users and different platforms when I know the build is likely to fail w/o them. We effectively have our own fork, but the goal is to get our working solutions back into spack proper. I want to tell folks they can build with spack without them having to become spack experts. I guess its a philosophy thing. In my experience being less specific and hopeful about dependencies always leads to failure.

The selections are setup to make the build work on a wide set of platforms (osx, general linux, and bgq), not just at LLNL. I guess I don't see how they are much different from depending on specific versions of libs, which has to be acceptable given API changes.

I know the host-config files look strange in the context of spack. An initial cache file is a standard feature of cmake, but it could be configured with all of those options via -D. We use the host-config files for reproducibility and automation outside of spack, so its important we create them.

davydden · 2016-12-23T16:06:45Z

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

+    # Python
+    #######################
+    depends_on("python", when="+python")
+    depends_on("py-numpy~blas~lapack", when="+python")


for the record, numpy builds fine for me on OS-X Sierra with the current develop (3f1cdbf). If it's still a problem, please file a separate bug report with detailed info. Whereas here py-numpy shall not be restricted to ~blas~lapack as it is NOT a requirement from the pakage, but a workaround to build py-numpy.

p.s. also check out this #2365

davydden · 2016-12-23T16:09:04Z

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

+    depends_on("hdf5~shared~cxx~mpi~fortran", when="~shared")
+
+    depends_on("silo+shared~fortran", when="+shared")
+    depends_on("silo~shared~fortran", when="~shared")


These are all due to various build issues, many on bgq ...

I agree with others and I am not in favour of over-constraining (~cxx~fortran) things only because something fails to build on some platforms. If there are issues, they need to be addressed separately.

I explicitly don't want MPI dependencies for HDF5 in Conduit

that's ok as long as it's a requirement of Conduit, not a build workaround.

citibeth · 2016-12-23T16:51:52Z

I am worried about the suggestions to rely on package.yaml to tweak things for different sets users and different platforms when I know the build is likely to fail w/o them.

Unfortunately, I do not know a better way. There are two problems with doing things the way they currently are in this PR:

The selections that have been added for particular platforms likely aren't what people need on other platforms. package.py files need to be platform-agnostic.
Even if you're willing to create a set of Spack packages specific to one platform, the "fixes" are being applied in the wrong place. If py-numpy has problems on OSX, then you should be fixing py-numpy/package.py. Fixing it in conduit/package.py is really the wrong place, and won't help anyone who needs to build py-numpy for any other reason.

I want to tell folks they can build with spack without them having to become spack experts.

I would recommend you develop a package.py file that makes it work on each platform / configuration that you need. I believe that Spack can do platform-specific configuration; and if it does not, we can/should extend it to do so. Once you get an OSX-specific and bgq-specific package.py files that you like, let's work to integrate them into Spack. That will give you the "out-of-box" functionality you want, while keeping the packages platform-agnostic. It can be a win-win for everyone.

I guess I don't see how they are much different from depending on specific versions of libs, which has to be acceptable given API changes.

When writing a package.py file, you put in it only things that are specific to that PACKAGE. If Conduit will only work with CMake 3.3.1, then you put depends_on('cmake@3.3.1'). If Conduit works just fine with any CMake 3 but CMake 3.4 and later don't build on OSX, then you put depends_on('cmake@3:') in conduit/package.py and specify in your OSX-specific packages.yaml to use cmake@:3.3.

Back to the question... when you depend on a specific version of a lib, you are doing so because the PACKAGE (on any plaftform) needs that version. Does this make sense?

I know the host-config files look strange in the context of spack. An initial cache file is a standard feature of cmake, but it could be configured with all of those options via -D. We use the host-config files for reproducibility and automation outside of spack, so its important we create them.

Does CMake generate this file when you run it with -D flags? Would it be acceptable for Spack to run CMake as usual, and then grab the CMakeCache.txt file for reproducibility outside of Spack?

I would also encourage you to read about spack setup in the manual. In that case, Spack generates a script (spconfig.py) that runs CMake with a particular environment and set of -D options. Once you've generated that file, you can build and install the package without Spack. I'd be interested in seeing how spack setup fits into your needs (and if any extensions to the idea would help it fit better).

adamjstewart · 2016-12-23T17:05:21Z

I would recommend you develop a package.py file that makes it work on each platform / configuration that you need. I believe that Spack can do platform-specific configuration; and if it does not, we can/should extend it to do so. Once you get an OSX-specific and bgq-specific package.py files that you like, let's work to integrate them into Spack.

Just to clarify, there are no platform-specific packages in Spack currently. We don't want one macOS package and one Linux package, we want a single package that can build on both. There are ways to do this. For example:

depends_on('py-numpy', when='+python')
depends_on('py-numpy~blas~lapack', when='+python platform=darwin')

should do what you're looking for. Elsewhere in the package you can do:

import sys
if sys.platform == 'darwin':
    # do macOS specific stuff

This should allow you to create a single package that works for all platforms. You can do the same for bgq as well. Although we should still try to resolve any build problems you have with other packages in the long run.

I know the host-config files look strange in the context of spack. An initial cache file is a standard feature of cmake, but it could be configured with all of those options via -D. We use the host-config files for reproducibility and automation outside of spack, so its important we create them.

Spack already has a lot of reproducibility features builtin. The entire cmake or configure line you run is recorded in the installation directory in .spack/build.out, so you'll always know how it was built. The .spack/spec.yaml file tells you exactly what dependencies your package was built with. You can always clone Spack and rebuild the same package in the same way. And it sounds like spack setup would be very useful for you as well. I guess I don't see any advantage of using a host-config file over specifying -D flags, which is what we do elsewhere.

citibeth · 2016-12-23T18:17:02Z

For example:

depends_on('py-numpy', when='+python')
depends_on('py-numpy~blas~lapack', when='+python platform=darwin')

should do what you're looking for.

@adamjstewart I believe that putting this in a OS-dependent packages.yaml would be far preferable, wouldn't you agree?

davydden · 2016-12-23T20:47:37Z

wouldn't you agree?

i would, especially given that py-numpy+blas+lapack actually builds on at least some macOS'es.

adamjstewart · 2016-12-24T00:10:13Z

It would be better. I'm just trying to compromise 😄

citibeth · 2016-12-24T06:11:29Z

It would be better. I'm just trying to compromise 😄

I see... Well I'm suggesting a different compromise that will be more work for "Core Spack" developers, but will ultimately push things forward... @cyrush gets things working with "plain vanilla" packages and OS-specific package.yaml files. He then submits the package.yaml files to this PR by attaching them to a comment. We incorporate those into Spack as platform-specific configuration files (see https://spack.readthedocs.io/en/latest/configuration.html#platform-specific-scopes ). I'm sure this will not be the last time we want OS-specific tweaks added to Spack.

adamjstewart · 2016-12-24T17:54:51Z

@citibeth At first I thought you were suggesting developing platform-specific package.py files, but if you just mean developing a generic package.py and platform-specific packages.yaml files, then I'm onboard with that.

citibeth · 2016-12-24T19:01:22Z

Yes, that is what I'm suggesting. Platform-specific `packages.yaml` files.

…

On Sat, Dec 24, 2016 at 12:54 PM, Adam J. Stewart ***@***.***> wrote: @citibeth <https://github.com/citibeth> At first I thought you were suggesting developing platform-specific package.py files, but if you just mean developing a generic package.py and platform-specific packages.yaml files, then I'm onboard with that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2670 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd8XlS5GJuMPvqNWOjTW-wYDvz-Qwks5rLVxsgaJpZM4LUahC> .

cyrush · 2016-12-27T19:08:59Z

Wow, lots of comments.

I understand the desire to leave things unconstrained. But practically this worries me.

The primary reason it worries me is that if I hadn't tested to identify what paths work, most of the conversation wouldn't have happened:

Stripping it back to basic dependencies is what I started with and it took a trivial amount effort to craft the initial package.py. Testing and finding the more specific paths took quite a bit of iteration and time. I like spack b/c it has the power to allow me to share those paths. My experience tells me that it is exactly finding these paths is where the bulk of the effort lies in proving a workable build for users across a wide range of platforms.

To be clear, I also would love for things to be unconstrained as spack evolves. I would love to be able to use the power of spack specs to help me test conduit against new versions of a TPL across platforms. But, the more basic need is for things to work first. Right now it seems we are trying to solve hypothetical problems (someone may want to build conduit against any version of a TPL, etc) at the expense of creating real problems (doesn't build out of the box on several platforms where were we have a need now).

For our local development setup, a packages.yaml will be perfect (we actually use project wide compilers.yaml files already), but helping craft a packages.yaml for spack proper (even just an os specific one) worries me b/c I am not the authority on those other packages. I would rather folks build a few more variations of a TPL to get conduit working, than dictate how things should work across a platform when conduit isn't even in the picture (read: >99.9999% of the builds).

As for the host-configs, there are several compelling reasons why we use them -- most of them unrelated to spack. For example, we revision control and share these files -- they are the ground truth for how things were configured. We often use spack to build TPLs, but don't use spack to build the project under development, etc. CMake can't generate these style of files. In other projects I have used dark CMake magic to try make it happen, but it always falls short. We can use spack can generate them for us, which is fantastic.

I don't want to have to fully elaborate / defend host-configs as part of this pull request. Seems like much to high a bar for the package.py.

I am going to try to attack a few more things (removing +shared in deps since it defaults to on, compiling with +doc vs +docs, etc) and we can continue from there.

tgamblin

Thanks everyone and @cyrush for iterating on this. In general since conduit is not widely used (yet) I don't have too many objections to how @cyrush constrains the package file, mainly because he wants this to work out of the box for his team.

I think we ought to take the various overly strict constraints as todos, fix them, and then try to loosen the constraints in conduit once the issues are resolved, not the other way around.

There are a few things I think I should note here.

@cyrush: you can actually set preferences just for conduit, just for specific platforms, in Spack's default packages.yaml. See concretization preferences. If you put them in /etc/spack/defaults/packages.yaml, users can still override but the settings will ship with Spack.
There isn't a way to express all the things @cyrush is trying to express in packages.yaml. In particular, you can't prefer that dependencies be built a certain way in packages.yaml when they're brought in by conduit. You can only add global settings for them. You can have a per-package providers settings, i.e. conduit could prefer openmpi for mpi. maybe we should just have a general dependency constraint section in packages.yaml in addition to providers.
packages.yaml has soft settings -- they settings there will be used if not otherwise constrained. package.py settings are hard settings -- Spack won't build if those constraints are violated. It seems like everyone might be happer if package.py settings were soft and if the user could override them.

Thoughts?

tgamblin · 2016-12-27T19:34:19Z

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

+    # Python
+    #######################
+    depends_on("python", when="+python")
+    depends_on("py-numpy~blas~lapack", when="+python")


@cyrush: I don't mind if you need these defaults for now but this should be a todo for us to make sure all the numpy variants (or at least the defaults) actually build properly on macOS. 1.0 in February will have package testing, so I think at that point at least you can remove the constraint.

change name of 'github-master' to 'master' change 'docs' variant to 'doc', set default to False remove explicit +shared variant spec for silo and hdf5 deps (in the conduit +shared case) cases since they default to True add reference to static rpath issue (#2658)

cyrush · 2016-12-27T20:31:34Z

Thanks @tgamblin, I would be happy to remove these constrains as the issues are resolved.

cyrush · 2016-12-27T20:34:27Z

Q: to use "extends" for python, what style of directory format does my python module need to live in?

I use pure lib installs, so I don't typically use the lib/pyhonX/site-packages/ hierarchy, but I have an option to name whatever destination is necessary, just need to know what spack's python support expects.

adamjstewart · 2016-12-28T01:10:14Z

@cyrush Take a look at the installation directories for py-numpy or any other Python package. Just use that format if you can.

citibeth · 2016-12-28T02:01:26Z

For our local development setup, a packages.yaml will be perfect (we actually use project wide compilers.yaml files already), but helping craft a packages.yaml for spack proper (even just an os specific one) worries me b/c I am not the authority on those other packages. I would rather folks build a few more variations of a TPL to get conduit working, than dictate how things should work across a platform when conduit isn't even in the picture (read: >99.9999% of the builds).

I see... Well, I'd look for your judgement on this one. For example, if you discover that:

numpy with the blas variant fails to build on my osx machine

Then I believe you should do some of the following, depending on how deep you want to get into the issue:

Disallow py-numpy+blas in an osx-specific packages.yaml file that ships with Spack. I'm comfortable with seeing that merged because you couldn't build it. Until someone discovers otherwise, I think it's best to disallow that combination on osx, thus steering Spack toward combinations that we know work. If you don't add this, OSX users will keep stumbling over the same pitfall again and again.
Ask on the Spack mailing list if anyone has had trouble building py-numpy+blas on other machines (or try it yourself). This will give some indication of whether this is an OSX-specific issue, or a wider bug in py-numpy (or BLAS).
When you add the appropriate line to the OSX-specific packages.yaml, add a comment pointing to the discussion thread where you mentioned your problems. This will allow others to retrace your steps if, at some point in the future, py-numpy+blas starts working.

Q: to use "extends" for python, what style of directory format does my python module need to live in?

@cyrush Spack enforces nothing on this issue (at least if you don't want to use spack activate, which I am not so familiar with). You just have to install your stuff somewhere and build the module with the right $PYTHONPATH, and it will work.

BUT... I highly recommend you ask Python for where it wants to put files. This requires that you run python, it's not something you can deduce statically. See my FindPython.cmake for how to do this: https://github.com/citibeth/icebin/blob/develop/cmake/FindPython.cmake You can also look at that project for an example of how to build a Python extension with CMake, not setup.py.

There isn't a way to express all the things @cyrush is trying to express in packages.yaml. In particular, you can't prefer that dependencies be built a certain way in packages.yaml when they're brought in by conduit. You can only add global settings for them. You can have a per-package providers settings, i.e. conduit could prefer openmpi for mpi. maybe we should just have a general dependency constraint section in packages.yaml in addition to providers.

If (for example) openmpi doesn't work on bgq but mpich does, then that preference should be expressed globally (in packages.yaml). That would be more useful than what @cyrush is currently expressing in package.py.

packages.yaml has soft settings -- they settings there will be used if not otherwise constrained. package.py settings are hard settings -- Spack won't build if those constraints are violated. It seems like everyone might be happer if package.py settings were soft and if the user could override them.

Thoughts?

If the constraints are there because a certain dependency only builds that way on bgq, then it should go in a bgq-specific packages.yaml file that we ship with Spack. That will not only make conduit work, it will also help other things work on bgq --- and will also not mess up efforts to build conduit on other machines.

If the constraints are there because conduit needs them (regardless of the machine it's running on), then they should remain in package.py.

If we don't yet know why the constraint is there... I think we should make a good guess.

cyrush · 2016-12-28T17:27:17Z

@adamjstewart @citibeth thanks -- for extends, just looking to support spack activate so I think adam's suggestion will help.

cyrush · 2016-12-28T17:35:26Z

On the packages.yaml vs package.py balance:

It sounds like there are a lot of trade offs. If I were more confident in my own understanding, I would aim for a packages.yaml solution to try to solve these issues in the other packages.

Given that I am not very versed with the new way spack operates (I have been using a frozen version for many many months), and this is my first attempt to update -- I think the safer thing seems to be for me to craft my package.py in a way that works and if I make any mistakes the fallout will be limited to my package. Maybe putting TODO comments in my package.py would help? Sure it would be hard for other folks to find, but at least it would be there for future ref.

citibeth · 2016-12-29T01:56:40Z

Yes... any comments explaining the reason for any restriction (or pointing to a web page explaining the reason) will be useful.

…

On Wed, Dec 28, 2016 at 12:35 PM, Cyrus Harrison ***@***.***> wrote: On the packages.yaml vs package.py balance: It sounds like there are a lot of trade offs. If I were more confident in my own understanding, I would aim for a packages.yaml solution to try to solve these issues in the other packages. Given that I am not very versed with the new way spack operates (I have been using a frozen version for many many months), and this is my first attempt to update -- I think the safer thing seems to be for me to craft my package.py in a way that works and if I make any mistakes the fallout will be limited to my package. Maybe putting TODO comments in my package.py would help? Sure it would be hard for other folks to find, but at least it would be there for future ref. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2670 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cdxT6DcWa71_ae3tENvUIdlzI8iHMks5rMp3fgaJpZM4LUahC> .

citibeth · 2016-12-29T04:30:38Z

or extends, just looking to support spack activate so I think adam's suggestion will help.

It looks like what I did and what Adam did might be orthogonal. A few observations:

I don't have a setup_dependent_package() in icebin/package.py, which leads me to believe it might not work with spack activate. I've never tested that. @adamjstewart Am I right to surmise that spack activate won't work without setup_dependent_package()?
In py-numpy/package.py, Python installs what it will (va setup_py()), and then the Spack package guesses where setup_py() installed it. If Python decides to install things differently / elsewhere for whatever reason, then this won't work. I don't know how likely that is. Better would be some way to query Python where it installed things.
Like your package, IceBin uses CMake (and doesn't use Python setup tools). The FindPython.cmake file goes to a lot of effort to let CMake install things in a Python-compatible way. But it (a) does not install in a Python egg (which is more complicated), and (b) does not tell Spack where it install things (but it could, for example, by writing out a little file in response to a Spack-specific CMake flag...). If it did that, then setup_dependent_package() could be built properly.

Conclusions...

It's an utter mess.
You will probably want to take stuff from both our examples.

adamjstewart · 2016-12-29T16:39:01Z

I don't have a setup_dependent_package() in icebin/package.py, which leads me to believe it might not work with spack activate. I've never tested that. @adamjstewart Am I right to surmise that spack activate won't work without setup_dependent_package()?

spack activate simply symlinks the installation directories into the installation directories of the package it extends. If you extend Python, this means symlinking things like bin and site-packages. If you'll notice, none of the Python packages define any setup_dependent_package, so no, you don't need to do that. See activate in Python's package.py for details.

add todos and more info on why variants for deps where selected use python module install python to enable spack activate use .format instead of %s

cyrush · 2017-01-03T19:39:19Z

@citibeth @adamjstewart @davydden @tgamblin

Addressed a few more concerns, by adding todos and moving to use extends for python.

I verified that spack activate does symlink my module into the proper place for the python install, however I can't actually import it when i run

bin/spack activate conduit 
==> Activated extension conduit@0.2.0%clang@7.3.0-apple+cmake~doc+hdf5+mpi+python+shared+silo arch=darwin-elcapitan-x86_64-prnyza2 for python@2.7.12~tk~ucs4%clang@7.3.0-apple

ls opt/spack/darwin-elcapitan-x86_64/clang-7.3.0-apple/python-2.7.12-pe3npk5rwg26kxq6fg4ybopjwg5fydfd/lib/python2.7/site-packages/
README						easy-install.pth
conduit						numpy-1.11.2-py2.7-macosx-10.6-x86_64.egg


bin/spack python
Spack version 0.9.1
Python 2.7.12, Darwin x86_64

>>> import conduit
Traceback (most recent call last):
  File "<console>", line 1, in <module>
ImportError: No module named conduit

That said, I have the same issue for numpy. It finds the wrong numpy, even though I have nothing python related set in my env (no PYTHONHOME or PYTHONPATH).

I have never actually used spack activate or bin/spack python before. From experience, I know that building a python that doesn't get polluted by a system python (or another python) is hard to accomplish. Their may be an issue with how python is built or with spack activate, but both are outside of the scope of this pr.

Please let me know if there is anything else I need to address.

tgamblin · 2017-01-03T21:56:54Z

IMHO we're good for now. I think we should merge this and work on the various issues this PR has brought up, and revisit Conduit as we make it easier to get reliable builds. Package testing in Spack v1.0 (end of Feb) will hopefully be a major help for that.

adamjstewart · 2017-01-03T22:01:50Z

I can't actually import it when i run
$ spack activate conduit
$ spack python
>>> import conduit

I think spack python just uses the system Python, not the Python conduit was linked to. If you instead run:

$ spack find -p python  # get the python that conduit was build with
$ /path/to/this/python
$ >>> import conduit

it should work.

cyrush · 2017-01-03T23:35:40Z

@adamjstewart thanks for clarifying -- that worked!

tgamblin · 2017-01-03T23:37:28Z

@davydden: are you ok with these changes?

davydden · 2017-01-04T01:30:46Z

LGTM

tgamblin · 2017-01-04T01:33:40Z

@davydden can you approve changes?

cyrush · 2017-01-04T18:18:22Z

thanks everyone for the feedback!

tgamblin · 2017-01-04T18:20:16Z

@cyrush: thanks for the package!

adrienbernede · 2020-04-16T17:33:10Z

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

+    # we are not using hdf5's mpi or fortran features.
+    depends_on("hdf5~cxx~mpi~fortran", when="+shared")
+    depends_on("hdf5~shared~cxx~mpi~fortran", when="~shared")
+
+    # we are not using silo's fortran features
+    depends_on("silo~fortran", when="+shared")
+    depends_on("silo~shared~fortran", when="~shared")


@cyrush @white238
I get that you are not using these features, but wouldn’t conduit work the same with this features?
If that is the case, I think for the sake of having as little constraint as possible of the spec we should no specify the mpi variant in hdf5.
This would allow to concretize and build a working version of conduit together with other products needing the mpi variant to be "on".
Does it makes sense to you?

@adrienbernede, yes, I am ok with relaxing.

Note: this type change to conduit is already part of the ceed PR:

https://github.com/spack/spack/pull/15500/files#r395829308

Maybe verify those changes look good?

Thanks! Looks great and already merged in Spack, wonderful!

cyrush added 2 commits December 22, 2016 13:57

add package for conduit

836d708

try to fix main conduit docstring

578db9b

adamjstewart reviewed Dec 22, 2016

View reviewed changes

use join_path instead of pjoin

867a954

citibeth reviewed Dec 23, 2016

View reviewed changes

davydden suggested changes Dec 23, 2016

View reviewed changes

tgamblin reviewed Dec 27, 2016

View reviewed changes

address pr #2670 requests

b7052a3

add todos and more info on why variants for deps where selected use python module install python to enable spack activate use .format instead of %s

davydden approved these changes Jan 4, 2017

View reviewed changes

tgamblin merged commit eaa24b0 into spack:develop Jan 4, 2017

cyrush deleted the feature/add_package_for_conduit branch January 4, 2017 18:21

tgamblin mentioned this pull request Feb 11, 2017

LBANN #3049

Merged

citibeth mentioned this pull request Feb 11, 2017

Projects vs. Packages: Principles for Spack Authorship and Use #3131

Closed

adrienbernede reviewed Apr 16, 2020

View reviewed changes


		version('0.2.0', 'd595573dedf55514c11d7391092fd760')

		version('github-master', git='https://github.com/LLNL/conduit.git')

add package for conduit #2670

add package for conduit #2670

Conversation

cyrush commented Dec 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyrush Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyrush commented Dec 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

citibeth commented Dec 23, 2016

cyrush commented Dec 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

citibeth commented Dec 23, 2016

adamjstewart commented Dec 23, 2016

citibeth commented Dec 23, 2016

davydden commented Dec 23, 2016

adamjstewart commented Dec 24, 2016

citibeth commented Dec 24, 2016

adamjstewart commented Dec 24, 2016

citibeth commented Dec 24, 2016 via email

cyrush commented Dec 27, 2016

tgamblin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyrush commented Dec 27, 2016

cyrush commented Dec 27, 2016

adamjstewart commented Dec 28, 2016

cyrush Dec 22, 2016 •

edited

Loading