PythonPackage: add import module smoke tests by adamjstewart · Pull Request #20023 · spack/spack

adamjstewart · 2020-11-20T20:02:39Z

This PR adds smoke tests for the 1,255 Python packages in Spack. It has been tested on the 180+ Python packages I have installed on my system.

Specifically, this PR makes the following changes:

Automatically detect import_modules
Run import module tests as smoke tests and install tests
Update existing packages that no longer need to set import_modules
Remove build_test and install_test phases
Update existing packages that use build_test and install_test
Remove no longer needed test deps
Update PythonPackage build system docs

Reboot of #15370
Closes #20079
Closes #20090

adamjstewart · 2020-11-21T02:06:58Z

lib/spack/spack/build_systems/python.py

-    def build_test(self):
-        """Run unit tests after in-place build.
-
-        These tests are only run if the package actually has a 'test' command.
-        """
-        if self._setup_command_available('test'):
-            args = self.test_args(self.spec, self.prefix)
-
-            self.setup_py('test', *args)


Running python setup.py test is deprecated, as most packages tell you to use something like tox instead. Also, the vast majority of packages do not upload any test files to PyPI, making these tests a no-op. Even if they do, I rarely manage to get the tests to pass, and many tests introduce circular dependencies. I've been meaning to do this for a long time now, but I'm removing these tests for the aforementioned reasons.

adamjstewart · 2020-11-21T02:08:29Z

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

    version('17.1', sha256='f019b770dd64e585a99714f1fd5e01c7a8f11b45635aa953fd41c689a657375b')
    version('16.8', sha256='5d50835fdf0a7edf0b55e311b7c887786504efea1177abd7e69329a8e5ea619e')

+    depends_on('py-setuptools', type='build')


I noticed a bug where spack install py-packaging would install a .egg-info file but no actual packaging.py file. This was causing other packages to fail. Adding a setuptools dependency fixes this bug.

Great find.

adamjstewart · 2020-11-21T21:55:59Z

@skosukhin may be interested in reviewing this.

vvolkl · 2020-11-25T17:45:00Z

Refers #20079

adamjstewart · 2020-11-30T20:31:23Z

Ping @becker33 @tldahlgren

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

tldahlgren · 2020-12-05T02:07:05Z

This PR affects a lot of files. Are you sure you haven't missed any? ;)

I haven't checked each package, but a basic ls py-*lists 1281 (1314 in develop) built-in packages in my clone. Grep'ing for built-in packages that inherit from PythonPackage lists 1228 packages, 22 of which do not start with py-.

FWIW. I plan to install a bunch of packages and run the tests.

tldahlgren · 2020-12-05T02:32:56Z

How did you decide on which (python) test dependencies to remove? I see a number of packages that still list test dependencies on packages such as py-mock (i.e., ambari, awscli), py-nose (e.g., awscli, nest, py-numpy, vigra), googletest (e.g., flann, msgpack-c, snappy, sumo) and others.

adamjstewart · 2020-12-05T04:55:10Z

I removed test dependencies from packages that no longer have unit tests.

tldahlgren · 2020-12-12T00:38:08Z

lib/spack/spack/build_environment.py

    elif context == 'test':
        import spack.user_environment as uenv  # avoid circular import
        env.extend(uenv.environment_modifications_for_spec(pkg.spec))
+        env.extend(


Hmm. This fix was integrated in #20298 . Unfortunately, it isn't quite enough in some cases (#20326 ).

lib/spack/spack/build_systems/python.py

tldahlgren · 2020-12-12T00:59:04Z

Closed by mistake.

tldahlgren · 2020-12-12T00:59:33Z

Reopening since accidentally closed.

tldahlgren

The tests appeared to pass for all of the packages I installed.

I would prefer to see output generated to the test logs along the lines produced by the run_test test method.

adamjstewart · 2020-12-12T04:35:52Z

@tldahlgren the latest commit makes use of run_test. I tested it for py-nose at both install time and as a post-installation test. As far as I'm concerned this should be good to go now.

tldahlgren

LGTM. I prefer explicit purpose descriptions for test but the test log output was sufficient.

I had a failure for one of the packages but that may be because I was having concretization issues with my last round of installs.

Update: FWIW. After re-running spack install on all the packages I was testing, all of the tests now work.

adamjstewart · 2020-12-14T18:28:22Z

What would be a good purpose description? Would you include the module name in the purpose?

tldahlgren · 2020-12-14T18:35:06Z

What would be a good purpose description? Would you include the module name in the purpose?

I do in general. The reason is it can be useful to grep for test: when you're debugging to see which tests at least started. It can be more relevant when there are a variety of tests being executed.

My inclination would be something along the lines
test: <package name-version>: checking import of <python module>.
But I do appreciate that being explicit can appear redundant when you're reviewing the test log.

adamjstewart · 2020-12-14T18:48:48Z

Done. I didn't add test: <package name-version>: because that seems like it should be added globally if that's something we want.

tldahlgren · 2020-12-14T18:58:14Z

Done. I didn't add test: <package name-version>: because that seems like it should be added globally if that's something we want.

I agree. What do you think @becker33 ?

BTW. Without test: my aforementioned greps won't pick up these test starts.

cosmicexplorer

One possibly-surprising pylab insertion, and one nonblocking question about checking for modules named test.

cosmicexplorer · 2020-12-16T16:50:21Z

lib/spack/docs/build_systems/sippackage.rst

-When you run ``spack install --test=root py-pyqt5``, Spack will attempt
-to import the ``PyQt5`` module after installation.
+These tests often catch missing dependencies and non-RPATHed
+libraries. Make sure not to add modules/packages containing the word


Great clarification!

Nonblocking: Would it be useful if I created a separate issue discussing checking for test in the added modules/packages? Does that occur in import_modules()? Or is that intrinsic somehow?

As of now, import_modules doesn't ignore test* modules. When I originally wrote this in #15370, I was using setuptools.find_packages, which has a tool for this. It would probably be good to add something similar to the default import_modules implementation. Do you want that to happen in this PR or would a follow-up PR be fine?

FWIW. I think a follow-up PR is fine (or preferred).

cosmicexplorer · 2020-12-16T16:55:47Z

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

+        'matplotlib.sphinxext', 'matplotlib.cbook', 'matplotlib.backends',
+        'matplotlib.backends.qt_editor', 'matplotlib.style',
+        'matplotlib.projections', 'matplotlib.testing',
+        'matplotlib.testing.jpl_units', 'pylab'


'pylab' appears to be new here. Where does that arise from?

In Python lingo, there are "modules" and "packages". "modules" are things like six, where the only thing that gets installed is a single six.py file in site-packages. "packages" are things like numpy, where there is no numpy.py, but instead there is a numpy/__init__.py with sub-directories containing code.

Previously, I was using setuptools.find_packages to get a list of import modules. find_packages only returns "packages", not "modules". matplotlib is one of those rare packages that mixes "modules" and "packages". The installation prefix has both matplotlib/__init__.py, mpl_toolkits/__init__.py, and pylab.py. My new code checks for both, which is why pylab was added.

cosmicexplorer · 2020-12-16T16:56:53Z

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

    version('17.1', sha256='f019b770dd64e585a99714f1fd5e01c7a8f11b45635aa953fd41c689a657375b')
    version('16.8', sha256='5d50835fdf0a7edf0b55e311b7c887786504efea1177abd7e69329a8e5ea619e')

+    depends_on('py-setuptools', type='build')


Great find.

cosmicexplorer · 2020-12-16T16:59:15Z

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

+                        python_paths.append(root)
+
+        pythonpath = ':'.join(python_paths)
+        env.prepend_path('PYTHONPATH', pythonpath)


Much easier to follow, thank you.

Should allow us to automatically check more modules and alleviate the need to create some private checks in different Jenkins CIs. Original commit message: PythonPackage: add import module smoke tests (spack#20023)

adamjstewart added python documentation Improvements or additions to documentation build-systems stand-alone-tests Stand-alone (or smoke) tests for installed packages labels Nov 20, 2020

adamjstewart requested a review from becker33 November 20, 2020 20:02

adamjstewart added the sip label Nov 20, 2020

This comment has been minimized.

Sign in to view

adamjstewart commented Nov 21, 2020

View reviewed changes

adamjstewart marked this pull request as ready for review November 21, 2020 21:50

tgamblin requested a review from tldahlgren November 24, 2020 06:50

adamjstewart mentioned this pull request Nov 25, 2020

[py-attrs] remove pytests to avoid circular dependency #20090

Closed

tldahlgren reviewed Dec 5, 2020

View reviewed changes

var/spack/repos/builtin/packages/py-black/package.py Show resolved Hide resolved

adamjstewart force-pushed the tests/python branch from 5e6e1fe to 918bd2e Compare December 7, 2020 18:25

tldahlgren reviewed Dec 12, 2020

View reviewed changes

lib/spack/spack/build_systems/python.py Show resolved Hide resolved

tldahlgren closed this Dec 12, 2020

tldahlgren reopened this Dec 12, 2020

tldahlgren requested changes Dec 12, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

adamjstewart added 2 commits December 11, 2020 22:20

PythonPackage: add import module smoke tests

f3f7ba7

spack test needs PYTHONPATH set for deps

4d8db2b

adamjstewart added 7 commits December 11, 2020 22:20

Remove import_modules from packages that no longer need it

ba1313e

package fixes

05089c1

More package updates

6464dd6

Remove unused test deps

2e71352

Update PythonPackage docs

86b57da

Update SIPPackage docs

8e45aa4

Use run_test method instead

39f1b70

adamjstewart force-pushed the tests/python branch from 918bd2e to 39f1b70 Compare December 12, 2020 04:33

tldahlgren approved these changes Dec 14, 2020

View reviewed changes

Add test purpose

4e6f3b5

cosmicexplorer approved these changes Dec 16, 2020

View reviewed changes

tldahlgren merged commit 826cd07 into spack:develop Dec 16, 2020

adamjstewart deleted the tests/python branch December 16, 2020 23:16

adamjstewart mentioned this pull request Dec 22, 2020

New package: py-pydmd #19809

Merged

bollig pushed a commit to bollig/spack that referenced this pull request Jan 12, 2021

PythonPackage: add import module smoke tests (spack#20023)

10b9996

adamjstewart mentioned this pull request Jan 17, 2021

R: add maintainer(s) #21119

Merged

loulawrence pushed a commit to loulawrence/spack that referenced this pull request Jan 19, 2021

PythonPackage: add import module smoke tests (spack#20023)

86fcb9e

matz-e mentioned this pull request Feb 4, 2021

cherry-pick: add import_module checks for Python modules BlueBrain/spack#1037

Closed

Conversation

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

Uh oh!

Uh oh!

This comment has been minimized.

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 Nov 21, 2020

Uh oh!

vvolkl commented Nov 25, 2020

Uh oh!

adamjstewart commented Nov 30, 2020

Uh oh!

Uh oh!

tldahlgren commented Dec 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tldahlgren commented Dec 5, 2020

Uh oh!

adamjstewart commented Dec 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tldahlgren commented Dec 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tldahlgren commented Dec 12, 2020

Uh oh!

tldahlgren left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

adamjstewart commented Dec 12, 2020

Uh oh!

tldahlgren left a comment • 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!

adamjstewart commented Dec 14, 2020

Uh oh!

tldahlgren commented Dec 14, 2020

Uh oh!

adamjstewart commented Dec 14, 2020

Uh oh!

tldahlgren commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cosmicexplorer 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adamjstewart commented Nov 20, 2020 •

edited

Loading

tldahlgren commented Dec 5, 2020 •

edited

Loading

tldahlgren commented Dec 12, 2020 •

edited

Loading

tldahlgren left a comment •

edited

Loading

tldahlgren commented Dec 14, 2020 •

edited

Loading