PythonPackage: Let There Be Tests! by adamjstewart · Pull Request #2869 · spack/spack

adamjstewart · 2017-01-18T22:26:21Z

Resolves #1305

The goal of this PR is to add a better testing framework for our Python packages. I see two low-hanging fruit that we can use for this:

Run python setup.py test if available
Try to import the Python module after installation

Most Python modules (maybe only the ones that use setuptools?) implement some kind of a "test" command that can be run after "build" and before "install". This provides basic package tests, implemented by the developers.

The main thing holding this up is that we really need a "test" deptype (see #1279). For example, py-setuptools has no dependencies, aside from Python. But the test suite requires:

tests_require=[                                                             
    'setuptools[ssl]',                                                      
    'pytest-flake8',                                                        
    'pytest>=2.8',                                                          
] + (['mock'] if sys.version_info[:2] < (3, 3) else []),

I certainly don't want to add these as build dependencies, because they will make all package installations significantly more complex. I only want to build these dependencies if the user supplies --run-tests. And I don't want these dependencies included in the hash. spack install --run-tests py-numpy and spack install py-numpy should not be two separate installations.

As for the import tests after installation, do we want to always run these (like the sanity_check_prefix method), or only with --run-tests? Here is what I'm envisioning. By default, Spack tries to import the name of the packages minus the "py-". We will also need a method to override this. For example, the import name for py-meep~mpi is meep, but the import name for py-meep+mpi is meep_mpi. This will need to be tested on a per-package basis, but for the most part, dropping "py-" should give us the import name for most Python packages.

adamjstewart · 2017-01-18T22:28:09Z

lib/spack/spack/build_systems/python.py

+        setup = self.setup_file(self.spec, self.prefix)
+
+        python(setup, '--no-user-cfg', command, '--help', **kwargs)
+        return python.returncode == 0


This seems kind of hacky, but I couldn't think of a better way to tell whether or not python setup.py test is available. I also don't like how it prints the test line when we run with spack install --verbose --run-tests. If we could make that configurable, it would be nice.

Not sure how best to do this, but note that by default Executables raise an exception on error. You need to pass fail_on_error=False

@tgamblin See kwargs above.

@adamjstewart: I think I'm past my daily intelligible github post limit 😄

citibeth · 2017-01-19T00:34:11Z

Everything in the first part of the OP sounds reasonable. I agree... we need a test deptype that doesn't change the hash. Should be possible, no?

As for the import tests after installation, do we want to always run these (like the sanity_check_prefix method), or only with --run-tests?

I would vote for always. Python packages aren't very useful if you can't import them, and it's MUCH quicker than real unit tests.

Alternately, could we have a new kind of test phase that does import tests?

Here is what I'm envisioning. By default, Spack tries to import the name of the packages minus the "py-".

My guess is that will lead to dozens or hundreds of spurious problems, which will have to be worked out immediately. There's not much variation here to make such an implicit rule useful, IMHO. I think we should KISS: the Spack package provides a list of Python packages to be imported on the test. Yes, this would mean we will only achieve test coverage over time. But I think it's a clean approach, and it makes the original PR MUCH easier.

Or do it right... root around in the package post-install, find all the packages, and try to import them.

Or do it really right... Python setup.py scripts contain the list of packages to install. Use setuptools/distutils to read that list directly, and then try your import tests.

adamjstewart · 2017-01-19T16:30:33Z

@citibeth Those are all really useful ideas that I didn't think about! I agree that we should probably always run import tests.

Let's look at the options we have:

The user must provide a list of modules to try importing

As you've probably already noticed, I prefer auto-magic over explicit if it cuts down on work and lines of code. But yes, if I made it auto-magic, I would have to manually test every PythonPackage in Spack to make sure it doesn't break anything. Not ideal. Asking the user to provide a list of modules and only running the import tests if this list exists is definitely the easiest way to go about things.

Root around in the package post-install, find all the packages, and try to import them

This could have unforeseen consequences. For example, we don't want to pick up modules that are only used for testing that would have additional dependencies. We also wouldn't want to pick up any "development" or "internal-use-only" files.

Python setup.py scripts contain the list of packages to install. Use setuptools/distutils to read that list directly, and then try your import tests.

Now this would be really nifty. I'm not sure how reliable it would be though. Some packages like setuptools do this, and we could get the list by running:

$ python
>>> import setup
>>> print(setup.setup_params['packages'])
['pkg_resources', 'setuptools', 'pkg_resources.extern', 'pkg_resources._vendor', 'pkg_resources._vendor.packaging', 'setuptools.extern', 'setuptools.command']

But setup.setup_params isn't any kind of standard convention. Other packages like numpy don't even do this.

I'm also seeing the following option:

$ python setup.py --help
...
  --provides          print the list of packages/modules provided

but this list is empty for all of the packages I've tried.

The way that py-setuptools gets its list is by running:

setuptools.find_packages(exclude=['*.tests'])

This might be the best option, although we run into two problems. Not all packages depend on setuptools, some use the builtin distutils, and I don't think distutils has this capability. Should we add setuptools as a dependency for all PythonPackages anyway? How do we prevent the dependency loop that would occur for py-setuptools depending on itself?

citibeth · 2017-01-19T16:36:59Z

Asking the user to provide a list of modules and only running the import tests if this list exists is definitely the easiest way to go about things.

I would suggest limiting the scope of this PR to that. Once the basic non-magic functionality is in, we can experiment with magic in a separate PR. Or maybe make more than one PR to experiment with the consequences of more than one kind of magic.

1. Root around in the package post-install, find all the packages, and try to import them This could have unforeseen consequences. For example, we don't want to pick up modules that are only used for testing that would have additional dependencies. We also wouldn't want to pick up any "development" or "internal-use-only" files.

Not sure this is a problem. Do Python modules install code in the prefix that is not intended to be used? Maybe they do...

Should we add setuptools as a dependency for all PythonPackages anyway?

No, it should only be a dependency for setuptools-dependent Python packages. Unless you want to make a SetuptoolsPackage and DistutilsPackage base class.

How do we prevent the dependency loop that would occur for py-setuptools depending on itself?

By not using a base class for `py-setuptools` that automagically depends on `py-setuptools`.

adamjstewart · 2017-01-19T17:17:30Z

Ok, I implemented the simplest case. If import_modules is defined in a package, Spack will try to import those modules after installation. I'm not particularly fond of the name, import_modules, so if anyone has any better suggestions, let me know.

adamjstewart · 2017-01-19T17:35:58Z

Should I run:

python -c 'import setuptools'

or:

python -c 'from setuptools import *'

I'm not sure if the latter will catch more problems. I'm pretty sure I've been able to import matplotlib but not able to import matplotlib.pyplot before.

citibeth · 2017-01-19T17:47:09Z

Would they not both be the same in terms of testing?

I'm pretty sure I've been able to import matplotlib but not able to

import matplotlib.pyplot before. No firm idea why. This is why the import tests you suggest are such a good idea!

…

On Thu, Jan 19, 2017 at 12:35 PM, Adam J. Stewart ***@***.***> wrote: Should I run: python -c 'import setuptools' or: python -c 'from setuptools import *' I'm not sure if the latter will catch more problems. I'm pretty sure I've been able to import matplotlib but not able to import matplotlib.pyplot before. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2869 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd-jNWrTnDaLUg3ODga8IHsheH3xiks5rT57_gaJpZM4LneCd> .

adamjstewart · 2017-01-20T18:59:51Z

@alalazo How do I override default tests? In PythonPackage, I essentially have:

@PackageBase.sanity_check('build')                                          
@PackageBase.on_package_attributes(run_tests=True)                          
def test(self):
    self.setup_py('test')

However, this doesn't work for numpy. In the py-numpy package, I tried overriding it with:

@PythonPackage.sanity_check('build')                                          
@PythonPackage.on_package_attributes(run_tests=True)                          
def test(self):
    pass

but it is still running python setup.py test. Any ideas?

alalazo · 2017-01-20T19:14:31Z

@adamjstewart In AutotoolsPackage and CMakePackage I call a function check indirectly, so that you can easily override just that. Maybe the right thing to do would be to push _run_default_function up the hierarchy, but I need to have a look how to do it cleanly. If you want I may try that in #2860.

adamjstewart · 2017-01-20T19:22:06Z

@alalazo I tried that too. With:

@PackageBase.sanity_check('build')                                          
@PackageBase.on_package_attributes(run_tests=True)                          
def test(self):
    ...

in PythonPackage and:

def test(self):
    pass

in py-numpy, it still tries to run the test in PythonPackage.

alalazo · 2017-01-20T19:25:52Z

@adamjstewart You miss indirection. In AutotoolsPackage I always call _run_default_function which does:

fn = getattr(self, 'check')

At that point you can override check everywhere in the hierarchy: the first match in the MRO of self is the one that gets called.

adamjstewart · 2017-01-20T19:46:07Z

Thanks @alalazo, I didn't notice that. I wonder if there's a better way to do that... Well, I can't think of one off the top of my head, so I'll steal your _run_default_function from AutotoolsPackage and roll with it.

adamjstewart · 2017-01-20T19:53:57Z

Now that I think about it, the easiest way to not have to use something like _run_default_function would be my suggestion here. I know you weren't too keen on it, but it might make our lives easier.

alalazo · 2017-01-20T19:56:17Z

@adamjstewart The easiest imho is to have only one version of _run_default_function in PackageBase, and register that function with the appropriate phase downstream in the hierarchy. At that point you just need a line in PythonPackage to use it.

adamjstewart · 2017-01-20T20:00:19Z

@alalazo Why did you do a try/except for that method? Was it before you added a default check method in AutotoolsPackage? It seems like some sort of check method will always exist.

alalazo · 2017-01-20T20:12:55Z

Why did you do a try/except for that method? Was it before you added a default check method in AutotoolsPackage?

I think your guess is right, if I remember well how it evolved. But I would leave it like it is: extra safety is always welcome! 😄

adamjstewart · 2017-01-20T22:02:12Z

lib/spack/spack/build_systems/python.py

+    # >>> import setuptools
+    # >>> setuptools.find_packages()
+    #
+    # in the source tarball directory


Is there some way to associate a docstring with a particular attribute in a class? I would like this information to show up in the API docs.

adamjstewart · 2017-01-20T22:05:57Z

var/spack/repos/builtin/packages/py-scipy/package.py

+        'scipy.sparse.linalg.eigen', 'scipy.sparse.linalg.isolve',
+        'scipy.sparse.linalg.eigen.arpack', 'scipy.sparse.linalg.eigen.lobpcg',
+        'scipy.special._precompute'
+    ]


Is there a better way to do this? This list is the output of:

>>> import setuptools >>> setuptools.find_packages()

adamjstewart · 2017-01-20T22:08:57Z

var/spack/repos/builtin/packages/py-scipy/package.py


+    # FIXME: Is this really necessary?
+    # Known not to work with 2.23, 2.25
+    # depends_on('[email protected]:', type='build')


We've discussed this before, but things like binutils and curl aren't really dependencies of any particular package. They are necessary for every package in Spack. In a way, they are Spack dependencies. The only reason people have been adding them is that certain packages require newer versions of these dependencies than are found on their ancient systems. We added a note about this in the documentation. I think this dependency should be removed. It won't even build on macOS.

adamjstewart · 2017-01-20T22:10:38Z

var/spack/repos/builtin/packages/py-scipy/package.py

+        #       scipy from its source directory; please exit the scipy
+        #       source tree, and relaunch your python interpreter from there.
+        with working_dir('..'):
+            python('-c', 'import scipy; scipy.test("full", verbose=2)')


By default, numpy and scipy run a quick test suite. If you pass the "full" label, they run the full test suite. This can take quite a long time (17 minutes for scipy on my Mac), but the good news is that ALL THE TESTS PASS! Should we run the full test suite or just the quick test suite? Neither will be run unless the user passes --run-tests anyway.

My vote is for full test suite if --run-tests is passed.

adamjstewart · 2017-01-20T22:14:38Z

var/spack/repos/builtin/packages/py-setuptools/package.py

+    # FIXME: Is there any way around this problem?
+    # depends_on('py-pytest-flake8', type='test')
+    # depends_on('[email protected]:', type='test')
+    # depends_on('py-mock', when='^python@:3.2', type='test')


One constraint I want to put on the 'test' deptype is that both of the following commands would install the same package with the same hash:

spack install py-setuptools spack install --run-tests py-setuptools

An unfortunate consequence of this is that we have no means of working around dependency loops. We can't do something similar to #2548 if we want to maintain this constraint, because the test dependency would have the same spec as the package that indirectly depends on it.

I don't think there is any good way around this problem. Let me now if you can think of anything better though.

P.S. The package tests that come with setuptools don't even pass, even when pip installs all of the required dependencies. The annoying thing about this is that if a user runs spack install --run-tests py-numpy, it will always fail when trying to build py-setuptools. Also, even if they did succeed, I'm worried that spack activate will copy these test dependencies to the Python installation, causing conflicts when trying to activate py-pytest or py-mock separately.

adamjstewart · 2017-02-20T14:46:09Z

@tgamblin I think this is ready for review. It won't work correctly until a test deptype is added, but it makes a lot of other modifications to numpy and scipy that I would like to get merged. Otherwise, I'll end up duplicating them in another PR.

adamjstewart · 2017-02-20T16:56:56Z

@alalazo I just updated this to use your new run_after('build')(PackageBase._run_default_build_time_test_callbacks) logic. Can you take a look and make sure I did it correctly? I wasn't able to test this as my db appears to be corrupted (or incompatible with this version of Spack).

adamjstewart · 2017-02-20T18:56:25Z

Moved some of the unrelated changes to #3195 to make this PR easier to review. I'll remove them from this PR in a minute.

adamjstewart · 2017-03-31T20:13:23Z

Ping @tgamblin. This PR won't be incredibly useful until we have a test deptype, but it shouldn't break anything if we merge this now. Then we'll at least have import tests for Python packages.

* Run python setup.py test if --run-tests * Attempt to import the Python module after installation * Add testing support to numpy and scipy * Remove duplicated comments * Update to new run-tests callback methodology * Remove unrelated changes for another PR

adamjstewart added python tests General test capability(ies) WIP labels Jan 18, 2017

adamjstewart commented Jan 18, 2017

View reviewed changes

Run python setup.py test if --run-tests

5aa9e5e

adamjstewart force-pushed the features/python-tests branch from 0f939c8 to 5aa9e5e Compare January 19, 2017 16:44

Attempt to import the Python module after installation

ccc15ad

Add testing support to numpy and scipy

91e326f

adamjstewart commented Jan 20, 2017

View reviewed changes

Remove duplicated comments

70546a0

adamjstewart commented Jan 20, 2017

View reviewed changes

alalazo mentioned this pull request Jan 21, 2017

build system modules: decorators are now free-standing (and renamed) #2860

Merged

4 tasks

adamjstewart mentioned this pull request Feb 8, 2017

python devmode #3079

Closed

Merge branch 'develop' into features/python-tests

d97c22b

adamjstewart changed the title ~~[WIP] PythonPackage: Let There Be Tests!~~ PythonPackage: Let There Be Tests! Feb 20, 2017

adamjstewart removed the WIP label Feb 20, 2017

Update to new run-tests callback methodology

7c506d9

adamjstewart mentioned this pull request Feb 20, 2017

Add new versions of several Python packages #3195

Merged

Remove unrelated changes for another PR

c99d2ef

This was referenced Feb 20, 2017

Add a test deptype #1279

Closed

Add package: git-review #3088

Merged

adamjstewart added 4 commits February 20, 2017 14:36

Fix a couple other bugs due to changes in build systems

00f91cd

Merge branch 'develop' into features/python-tests

c435627

Fix bad merge

d033a86

Fix bad indentation from merge

08f1015

adamjstewart mentioned this pull request Feb 21, 2017

Unable to build Numpy with Intel 17 #3204

Closed

This was referenced Mar 2, 2017

add package py-elephant #3302

Merged

Add package py-guiqwt and dependencies #3334

Merged

Merge branch 'develop' into features/python-tests

5e22942

adamjstewart mentioned this pull request Mar 29, 2017

add package py-brian2 #3617

Merged

tgamblin approved these changes Mar 31, 2017

View reviewed changes

tgamblin merged commit bc40453 into spack:develop Mar 31, 2017

adamjstewart deleted the features/python-tests branch March 31, 2017 20:48

adamjstewart mentioned this pull request Apr 1, 2017

EasyBuild and dependencies #3658

Merged

Conversation

adamjstewart commented Jan 18, 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!

citibeth commented Jan 19, 2017

Uh oh!

adamjstewart commented Jan 19, 2017

Uh oh!

citibeth commented Jan 19, 2017 via email

Uh oh!

adamjstewart commented Jan 19, 2017

Uh oh!

adamjstewart commented Jan 19, 2017

Uh oh!

citibeth commented Jan 19, 2017 via email

Uh oh!

adamjstewart commented Jan 20, 2017

Uh oh!

alalazo commented Jan 20, 2017

Uh oh!

adamjstewart commented Jan 20, 2017

Uh oh!

alalazo commented Jan 20, 2017

Uh oh!

adamjstewart commented Jan 20, 2017

Uh oh!

adamjstewart commented Jan 20, 2017

Uh oh!

alalazo commented Jan 20, 2017

Uh oh!

adamjstewart commented Jan 20, 2017

Uh oh!

alalazo commented Jan 20, 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!

alalazo Jan 21, 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!

adamjstewart commented Feb 20, 2017

Uh oh!

adamjstewart commented Feb 20, 2017

Uh oh!

adamjstewart commented Feb 20, 2017

Uh oh!

adamjstewart commented Mar 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adamjstewart commented Jan 18, 2017 •

edited

Loading

alalazo Jan 21, 2017 •

edited

Loading