Flann package by svenevs · Pull Request #3966 · spack/spack

svenevs · 2017-04-24T00:27:59Z

In general it seems to be working very well. Notes:

python3 is definitively broken. They have a weird cmake + setup.py thing going on.
- Not sure if extends('python') is correct. The site-packages directory is always empty.
- python2 works. Something about this does that
Defaults are +mpi and +hdf5 under the assumption that if anybody actually wants this library directly, they likely will want it for these purposes. The tests are all using these features.
- +openmp for same reasons.
Untested variants: matlab, didn't try and combine every possible variant either.
Not sure about the type for the py-numpy dependency. That's only needed for when somebody uses the python bindings, not the library itself.

What is the spack default cmake build type?

+        # Default is RelWithDebugInfo
+        args.append("-DCMAKE_BUILD_TYPE:STRING=Release")

Python2 somehow works. Python3 does not. Still debugging their obscure setup.py configuration.

Flake8 checks in place. Unsure about CMAKE_BUILD_TYPE and default spack behavior.

tgamblin · 2017-04-24T00:44:04Z

1: Does the python in Flann just not work with Python 3? If so it should probably extends('python') and depends_on('python@:2.8').

5: you can override build_type() for that.

svenevs · 2017-04-24T00:55:23Z

1: Does the python in Flann just not work with Python 3? If so it should probably extends('python') and depends_on('python@:2.8').

Well. They claim it does, and this person got it working, so it should be possible. I just can't figure out how to directly call python{2,3} setup.py directly. The cmake file linked above should be using the right python executable...

5: you can override build_type() for that.

Fixed. The cmake they have uses RelWithDebInfo as the default too. I guess I personally always default to Release, but this makes more sense so that people can still debug through their dependencies if needbe.

tgamblin · 2017-04-24T01:37:08Z

1: Does the python in Flann just not work with Python 3? If so it should probably extends('python') and depends_on('python@:2.8').

Well. They claim it does, and this person got it working, so it should be possible. I just can't figure out how to directly call python{2,3} setup.py directly. The cmake file linked above should be using the right python executable...

I would try passing this to CMake:

'-DPYTHON_EXECUTABLE=%s' % spec['python'].command.path

using @adamjstewart's #3367. Then you don't have to call setup.py -- you can let the cmake build call the right setup.py and run it using the right python. I would also patch this line to add some arguments to setup.py. If you want to do it exactly like PythonPackage would, make the call to setup.py there look like:

python setup.py --no-user-cfg --prefix={prefix}

--no-user-cfg prevents settings in home directories from interfering with the setup.py arguments. Finally, this build doesn't appear to use setuptools, but if it did, I would say to also add --single-version-externally-manager --root=/ per #3587.

So, basically, just add --no-user-cfg and a proper prefix. The lack of a prefix arg is probably why your site-packages directory was previously empty -- by default setup.py installs things to the site-packages directory of whatever Python it's called with.

adamjstewart · 2017-04-24T17:31:18Z

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

+    NOTE: The author of the package was careful to support HDF5 without
+    parallel support, so the dependencies do not require this, but you will
+    likely want to make sure that you are giving hdf5+mpi as the dependency,
+    this is the spack default so it should be fine.


I don't think this NOTE is necessary. This is the case for pretty much every package in Spack that comes with optional dependencies. As long as the variant defaults are reasonable, it's up to the user to explicitly deactivate this optional support.

Yes, I wrote that before deciding to change the defaults. Will remove

adamjstewart · 2017-04-24T17:32:10Z

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

+    depends_on("matlab",   when="+matlab")
+    depends_on("cuda",     when="+cuda")
+    depends_on("mpi",      when="+mpi")
+    depends_on("hdf5",     when="+hdf5")


Perhaps it should read:

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

I don't understand. Then you cannot do just mpi right?

No, you can still do +mpi. You would read these lines as:

If +hdf5 is in the spec, we need to depend on hdf5.
If +hdf5 and +mpi are in the spec, more specifically, we need hdf5+mpi.

These constraints are all merged into one, so there's no problem with specifying multiple depends_on statements for the same package to make it more specific.

I mean honestly I don't care, but it's not a hard requirement. I just don't understand why you want to make it more restrictive than it needs to be. The boost thing below is what accounts for them coupled together, no?

I just don't understand why you want to make it more restrictive than it needs to be.

I guess it depends on how restrictive it needs to be. If you can build flann+hdf5+mpi ^hdf5~mpi and it works, then it doesn't need to be that restrictive. But if flann+hdf5+mpi requires hdf5+mpi, then that's how you would do it.

It does not require that, it doesn't need mpi or hdf5. It can build with one or the other, and it can build with hdf5~mpi and +mpi. So I'll leave it?

I guess this comes down to what the defaults should be. Personally, I think the default should be to give the user the best possible version (within reason, e.g. minus cuda and bindings). So is there a way to make the default be +mpi and +hdf5 where hdf5+mpi is used?

If flann defaults to +hdf5+mpi and hdf5 defaults to +mpi, then it will do what you want. Of course, users can override all of these defaults in their packages.yaml. But if there is no hard requirement, than I agree, keep it as flexible as possible.

adamjstewart · 2017-04-24T17:33:14Z

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

+    depends_on("hdf5",     when="+hdf5")
+    # HDF5_IS_PARALLEL actually comes from hdf5+mpi
+    # https://github.com/mariusmuja/flann/blob/06a49513138009d19a1f4e0ace67fbff13270c69/CMakeLists.txt#L108-L112
+    depends_on("boost+mpi+system+serialization+thread", when="+mpi ^hdf5+mpi")


Just to clarify, this package doesn't require boost unless you're building with MPI support, with HDF5 support, and with HDF5_IS_PARALLEL? So if you're building +hdf5~mpi or ~hdf5+mpi you don't need boost at all?

That is my understanding of this section, which is the only place boost shows up.

adamjstewart · 2017-04-24T17:34:33Z

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

+
+class Flann(CMakePackage):
+    """
+    FLANN is a library for performing fast approximate nearest neighbor


Travis doesn't seem to be very happy today. I'll try to kick it off again. I'm going to predict that the documentation tests will fail for this package though. Sphinx doesn't like it when your docstring starts with a newline character.

Maybe you're sphinx doesn't. I forget where but one of the newer PEP said they became equivalent. I'll update em

Technically it's a valid docstring. The problem is that we generate a Package List based on these docstrings. If you run spack list --format=rst, you'll see what it looks like. If you take the last package in the list (zstd) and add a newline to the start of the docstring, you'll see that it prints:

Description: Zstandard, or zstd as short version, is a fast lossless compression algorithm, targeting real-time compression scenarios at zlib-level and better compression ratios.

The newline causes the indentation to be off by one space. I would have to look at the code that does this. Perhaps we could strip leading and trailing newline characters from the docstring before printing.

I will fix it to appease the spack gods. I might look into that, because I wanted to include rst directives in something once for spack but the info command totally chokes on it. It doesn't even preserve paragraph endings. I was going to be doing something over the summer related to running sphinx to acquire manpage output from rst (e.g. for tables, directives, etc). If I ever get it I'll ping you back and see if spack can benefit.

adamjstewart · 2017-04-24T17:34:55Z

Closing and reopening to try to kick off Travis.

svenevs · 2017-04-24T18:29:40Z

@tgamblin excellent, I suspect this will work! I will try it after my meetings ~7pm EST :) You were completely correct, the installation was just getting dumped into the python directories directly. I will check it out and make sure things are kosher.

Update: I'm waiting for 3367 to get merged in / an extremely large cookie got put on my plate unexpectedly. I should be able to test this out over the weekend, but I only want to do one more complete redo of spack + python. Adam's changes change the hashes (grrr), and I've been selectively merging things in that avoid recompiling it all. Out of curiosity, though, given it's nature, will 3367 get pulled into the 2548 beast?

In other words, do you have any suggested workflow here? It's getting really difficult to understand whether or not my PRs are even functional with develop at this point, because in order for me to function I have to merge in 2548, 3367, and obsessively find every possible depends_on("setuptools") and change the deps to be type=("build", "link"). It's time consuming, and results in me not being able to verify the legitimacy of what I've written.

To be clear, I'm not complaining at all! I just wonder if there's an easier way to do this other than rage sed replacing things...

svenevs · 2017-04-24T18:42:53Z

Here let's try closing this PR and see if the boost one goes through

svenevs · 2017-05-09T23:13:14Z

Ok so the latest commit works in the sense that it correctly builds things (thanks @tgamblin !!!) for both python 2 and python 3, and I can import pyflann for both. It's not a clean installation now, though.

sven:/opt/spack> source <(spack module loads --dependencies flann)                                       
sven:/opt/spack> python3
Python 3.6.1 (default, May  8 2017, 14:55:30) 
[GCC 5.3.1 20160406 (Red Hat 5.3.1-6)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pyflann
>>> pyflann.__path__
['/opt/spack/opt/spack/linux-fedora23-x86_64/gcc-5.3.1/python-3.6.1-vtgnau5wyennlx7g64w6zc6fk6jnq2ep/lib/python3.6/site-packages/pyflann']

so when I spack uninstall...pyflann still hangs around.

~~I'm gonna try the opencv approach of extends('python', when='+python') and see what happens.~~ Ugh nevermind that's already there. I'll try the direct setup.py approach then.

vvv yeah totally missed what Todd was saying...

adamjstewart · 2017-05-09T23:19:07Z

You need a patch to filter the CMake file that calls setup.py so that it adds --prefix. See #3966 (comment)

svenevs · 2017-05-10T01:05:38Z

The gods are good. I think it works as expected now, activate and all!

adamjstewart · 2017-05-10T13:02:12Z

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

+                    "src/python/CMakeLists.txt")
+        # Fix the install location so that spack activate works
+        filter_file("share/flann/python",
+                    "{0}".format(site_packages_dir),


You probably don't need to do this string formatting. site_packages_dir is already a string.

OK for the love of god man WHERE THE EFF DOES THIS COME FROM I'LL DOCUMENT IT NOW

Shh, it's a secret...

adamjstewart · 2017-05-10T13:04:40Z

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

+    def patch(self):
+        # Fix up the python setup.py call inside the install(CODE
+        filter_file("setup.py install",
+                    'setup.py install --no-user-cfg --prefix=\\"{0}\\"'.format(


If I remember correctly, python setup.py --help shows --no-user-cfg as an option and python setup.py install --help shows --prefix as an option. I would switch the order to setup.py --no-user-cfg install --prefix just to be safe.

https://spack.readthedocs.io/en/latest/spack.html#module-spack.package

search PythonPackage. how is this being generated / why are your class links gone from sphinx?

adamjstewart · 2017-05-11T20:48:20Z

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

+            description="Build the Python bindings. "
+                        "Module: pyflann. "
+                        "Python2 verified. "
+                        "Python3 claimed, not attainable.")


You can remove these extra comments now that we got Python 3 working. Build the Python bindings is enough.

adamjstewart · 2017-05-11T20:50:24Z

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

+    def patch(self):
+        # Fix up the python setup.py call inside the install(CODE
+        filter_file("setup.py install",
+                    '--no-user-cfg setup.py install --prefix=\\"{0}\\"'.format(


See #4221, this should actually be setup.py --no-user-cfg, not --no-user-cfg setup.py.

adamjstewart · 2017-05-11T20:52:34Z

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

+                    "# install( FILES",
+                    "src/python/CMakeLists.txt", string=True)
+
+    # Tests: require hdf5 and gtest, don't know what to do so ignore...


For the record, we're working on this. See #1279. I've been adding things like:

# TODO: Add a 'test' deptype # depends_on('hdf5', type='test') # depends_on('gtest', type='test')

to packages where I noticed there was a test dependency.

adamjstewart · 2017-05-11T20:54:33Z

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

+        spec = self.spec
+        args = []
+        # Default is RelWithDebugInfo
+        args.append("-DCMAKE_BUILD_TYPE:STRING=Release")


You should override build_type for this. Otherwise, we will end up specifying two different flags for cmake. Remove this line and add this definition above cmake_args:

def build_type(self): return 'Release'

This is one of the flags that are magically added to every CMakePackage. See spack edit -b cmake for details.

^ yeah my bad i totally meant to take that out but forgot, spack defaults to release with debug info, which is what they do in flann, so i just removed it

Unless you prefer the default, which is RelWithDebInfo.

yeah the default should remain, both spack and flann use it with good reason. if anybody develops with the library, no debug information makes life very difficult if / when you need to step through their functions.

adamjstewart · 2017-05-11T21:43:03Z

Everything looks good to me now. If you think it's safe to merge, just remove the [WIP] from the title.

svenevs · 2017-05-11T21:47:13Z

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

+    # Dependencies
+    extends("python",      when="+python")
+    depends_on("py-numpy", when="+python", type=("build", "run"))
+    depends_on("matlab",   when="+matlab")


wait @adamjstewart should this also be build + run for matlab?

Oh, um, probably. I've never actually built a matlab extension before. Hell, I'm not even sure if you can have multiple extensions (what does spack activate flann do, activate both?). There used to be a restriction on this but I commented it out because I didn't like it 😄

so. what? are you saying conflicts("+python", when="+matlab")? i don't have extends("matlab", when="+matlab") but I can't even test the matlab stuff because I don't have it and refuse to pay for it. so i guess i'll just leave it?

Yeah, just leave it. Probably best to add type=('build', 'run'). I don't think it links to the matlab libraries, probably just provides matlab scripts to run.

* Initial attempt at flann packaging. Python2 somehow works. Python3 does not. Still debugging their obscure setup.py configuration. * Flann good enough. Python3 broken but close. Flake8 checks in place. Unsure about CMAKE_BUILD_TYPE and default spack behavior. * spack uses RelWithDebInfo as default build type. * builds py2/3, but direct site-packages install * prefix working, empty python install dir * flann package +python installs correctly * str format {0} instead of {} * potential doctest fix * consistency of build env with PythonPackage * fix python again, test deptype todo, build type * potentially enable matlab, untested

svenevs added 2 commits April 23, 2017 19:03

Initial attempt at flann packaging.

9690ed5

Python2 somehow works. Python3 does not. Still debugging their obscure setup.py configuration.

Flann good enough. Python3 broken but close.

7617377

Flake8 checks in place. Unsure about CMAKE_BUILD_TYPE and default spack behavior.

spack uses RelWithDebInfo as default build type.

0f096ae

adamjstewart reviewed Apr 24, 2017

View reviewed changes

adamjstewart closed this Apr 24, 2017

adamjstewart reopened this Apr 24, 2017

adamjstewart mentioned this pull request Apr 24, 2017

Mark 1.64.0 unstable until +mpi+python fixed. #3964

Closed

svenevs closed this Apr 24, 2017

alalazo reopened this Apr 25, 2017

adamjstewart mentioned this pull request Apr 25, 2017

Travis builds not working for specific user travis-ci/travis-ci#7662

Closed

adamjstewart added the new-package label Apr 28, 2017

builds py2/3, but direct site-packages install

ca846ee

prefix working, empty python install dir

100f9aa

svenevs added 2 commits May 9, 2017 22:06

flann package +python installs correctly

66b8a81

str format {0} instead of {}

49c401e

svenevs mentioned this pull request May 10, 2017

[WIP] Add pcl package and friends, first attempt. #4188

Closed

15 tasks

potential doctest fix

fc5acdd

adamjstewart reviewed May 10, 2017

View reviewed changes

consistency of build env with PythonPackage

4735c95

adamjstewart reviewed May 11, 2017

View reviewed changes

adamjstewart mentioned this pull request May 11, 2017

Fix typo in PythonPackage documentation #4221

Merged

adamjstewart reviewed May 11, 2017

View reviewed changes

fix python again, test deptype todo, build type

b4c4359

adamjstewart approved these changes May 11, 2017

View reviewed changes

svenevs changed the title ~~[WIP] Flann package~~ Flann package May 11, 2017

svenevs commented May 11, 2017

View reviewed changes

potentially enable matlab, untested

e9cb34f

svenevs closed this May 11, 2017

svenevs reopened this May 11, 2017

adamjstewart merged commit d01f01c into spack:develop May 12, 2017

svenevs deleted the flann branch May 12, 2017 00:29

svenevs mentioned this pull request Sep 1, 2017

Set default cmake build_type to 'Release' for llvm #5261

Merged

Conversation

svenevs commented Apr 24, 2017

Uh oh!

tgamblin commented Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

svenevs commented Apr 24, 2017

Uh oh!

tgamblin commented Apr 24, 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!

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!

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!

adamjstewart commented Apr 24, 2017

Uh oh!

svenevs commented Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

svenevs commented Apr 24, 2017

Uh oh!

svenevs commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented May 9, 2017

Uh oh!

svenevs commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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

tgamblin commented Apr 24, 2017 •

edited

Loading

svenevs commented Apr 24, 2017 •

edited

Loading

svenevs commented May 9, 2017 •

edited

Loading

svenevs commented May 10, 2017 •

edited

Loading

svenevs May 11, 2017 •

edited

Loading