Skip to content

By default, don't include tests in binary distributions. #7757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 7, 2017

Conversation

StephanErb
Copy link
Contributor

@StephanErb StephanErb commented Jan 6, 2017

Following the discussion in #7745, I have changed the default for the packaging of tests.

Before this patch:

$ ls -lh dist
total 65M
-rw-r--r-- 1 vagrant vagrant 32M Jan  6 22:56 matplotlib-2.0.0rc2+2914.g1fa4dd7-cp27-cp27mu-linux_x86_64.whl
-rw-r--r-- 1 vagrant vagrant 33M Jan  6 22:53 matplotlib-2.0.0rc2+2914.g1fa4dd7.tar.gz

With this patch:

$ ls -lh dist
total 43M
-rw-r--r-- 1 vagrant vagrant 11M Jan  6 23:03 matplotlib-2.0.0rc2+2914.g1fa4dd7.dirty-cp27-cp27mu-linux_x86_64.whl
-rw-r--r-- 1 vagrant vagrant 33M Jan  6 23:02 matplotlib-2.0.0rc2+2914.g1fa4dd7.dirty.tar.gz

Before this patch:

    $ ls -lh dist
    total 65M
    -rw-r--r-- 1 vagrant vagrant 32M Jan  6 22:56 matplotlib-2.0.0rc2+2914.g1fa4dd7-cp27-cp27mu-linux_x86_64.whl
    -rw-r--r-- 1 vagrant vagrant 33M Jan  6 22:53 matplotlib-2.0.0rc2+2914.g1fa4dd7.tar.gz

With this patch:

    $ ls -lh dist
    total 43M
    -rw-r--r-- 1 vagrant vagrant 11M Jan  6 23:03 matplotlib-2.0.0rc2+2914.g1fa4dd7.dirty-cp27-cp27mu-linux_x86_64.whl
    -rw-r--r-- 1 vagrant vagrant 33M Jan  6 23:02 matplotlib-2.0.0rc2+2914.g1fa4dd7.dirty.tar.gz
@tacaswell tacaswell modified the milestones: 2.0.1 (next bug fix release), 2.0 (style change major release) Jan 7, 2017
tacaswell
tacaswell previously approved these changes Jan 7, 2017
@tacaswell
Copy link
Member

'power cycled' to restart appveyor.

@tacaswell tacaswell closed this Jan 7, 2017
@tacaswell tacaswell reopened this Jan 7, 2017
@tacaswell tacaswell dismissed their stale review January 7, 2017 04:40

Because I thought of another concern.

@tacaswell
Copy link
Member

One issue with this change is that it breaks tests on not-develop-installed source installations by default, ex

23:45 $ pip install .
Processing /home/tcaswell/source/p/matplotlib
Requirement already satisfied: numpy>=1.7.1 in /tmp/mpl_test/lib/python3.5/site-packages (from matplotlib==2.0.0rc2+2915.gda439c0cb)
Requirement already satisfied: six>=1.10 in /tmp/mpl_test/lib/python3.5/site-packages (from matplotlib==2.0.0rc2+2915.gda439c0cb)
Requirement already satisfied: python-dateutil in /tmp/mpl_test/lib/python3.5/site-packages (from matplotlib==2.0.0rc2+2915.gda439c0cb)
Requirement already satisfied: pytz in /tmp/mpl_test/lib/python3.5/site-packages (from matplotlib==2.0.0rc2+2915.gda439c0cb)
Requirement already satisfied: cycler>=0.10 in /tmp/mpl_test/lib/python3.5/site-packages (from matplotlib==2.0.0rc2+2915.gda439c0cb)
Requirement already satisfied: pyparsing!=2.0.4,!=2.1.2,!=2.1.6,>=1.5.6 in /tmp/mpl_test/lib/python3.5/site-packages (from matplotlib==2.0.0rc2+2915.gda439c0cb)
Installing collected packages: matplotlib
  Running setup.py install for matplotlib ... done
Successfully installed matplotlib-2.0.0rc2+2915.gda439c0cb
(mpl_test) ✔ ~/source/p/matplotlib [packaging_size {StephanErb/packaging_size}|✔] 
23:45 $ python tests.py 
Python byte-compilation optimization level: 0
Traceback (most recent call last):
  File "tests.py", line 46, in <module>
    success = test(argv=sys.argv[0:1] + extra_args, switch_backend_warn=False)
  File "/tmp/mpl_test/lib/python3.5/site-packages/matplotlib/__init__.py", line 1582, in test
    _init_tests()
  File "/tmp/mpl_test/lib/python3.5/site-packages/matplotlib/__init__.py", line 1554, in _init_tests
    raise ImportError("matplotlib test data is not installed")
ImportError: matplotlib test data is not installed
(mpl_test) ✘-1 ~/source/p/matplotlib [packaging_size {StephanErb/packaging_size}|✔] 

vs

23:46 $ pip install -e .
Obtaining file:///home/tcaswell/source/p/matplotlib
Requirement already satisfied: numpy>=1.7.1 in /tmp/mpl_test/lib/python3.5/site-packages (from matplotlib==2.0.0rc2+2915.gda439c0cb)
Requirement already satisfied: six>=1.10 in /tmp/mpl_test/lib/python3.5/site-packages (from matplotlib==2.0.0rc2+2915.gda439c0cb)
Requirement already satisfied: python-dateutil in /tmp/mpl_test/lib/python3.5/site-packages (from matplotlib==2.0.0rc2+2915.gda439c0cb)
Requirement already satisfied: pytz in /tmp/mpl_test/lib/python3.5/site-packages (from matplotlib==2.0.0rc2+2915.gda439c0cb)
Requirement already satisfied: cycler>=0.10 in /tmp/mpl_test/lib/python3.5/site-packages (from matplotlib==2.0.0rc2+2915.gda439c0cb)
Requirement already satisfied: pyparsing!=2.0.4,!=2.1.2,!=2.1.6,>=1.5.6 in /tmp/mpl_test/lib/python3.5/site-packages (from matplotlib==2.0.0rc2+2915.gda439c0cb)
Installing collected packages: matplotlib
  Found existing installation: matplotlib 2.0.0rc2+2915.gda439c0cb
    Uninstalling matplotlib-2.0.0rc2+2915.gda439c0cb:
      Successfully uninstalled matplotlib-2.0.0rc2+2915.gda439c0cb
  Running setup.py develop for matplotlib
Successfully installed matplotlib
(mpl_test) ✔ ~/source/p/matplotlib [packaging_size {StephanErb/packaging_size}|✔] 
23:46 $ python tests.py 
Python byte-compilation optimization level: 0
matplotlib.test requires nose and mock to run.
Traceback (most recent call last):
  File "tests.py", line 46, in <module>
    success = test(argv=sys.argv[0:1] + extra_args, switch_backend_warn=False)
  File "/home/tcaswell/source/p/matplotlib/lib/matplotlib/__init__.py", line 1582, in test
    _init_tests()
  File "/home/tcaswell/source/p/matplotlib/lib/matplotlib/__init__.py", line 1577, in _init_tests
    check_deps()
  File "/home/tcaswell/source/p/matplotlib/lib/matplotlib/testing/nose/__init__.py", line 22, in check_deps
    import nose
ImportError: No module named 'nose'
(mpl_test) ✔ ~/source/p/matplotlib [packaging_size {StephanErb/packaging_size}|✔] 
23:46 $ python tests.py 
Python byte-compilation optimization level: 0
..................................

This may break the workflows of a number of developers (attn @efiring ).

On the whole I am 👍 on this, but want to hear from others.

I am 70/30 on this being a 2.0 vs 2.1 change.

@codecov-io
Copy link

Current coverage is 62.12% (diff: 100%)

Merging #7757 into master will not change coverage

@@             master      #7757   diff @@
==========================================
  Files           174        174          
  Lines         56028      56028          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          34805      34805          
  Misses        21223      21223          
  Partials          0          0          

Powered by Codecov. Last update 1fa4dd7...da439c0

@efiring
Copy link
Member

efiring commented Jan 7, 2017

I've been using pip install -e . for a while now, so if I understand you correctly this change won't suddenly break my ability to run tests locally. I agree with the basic ideas of leaving the tests out of the wheels, and of not having a default setup.cfg. If putting this change in 2.0 makes the launch smoother, then I'm for it.

@dopplershift
Copy link
Contributor

I'm 👍 on this if for no other reason than selfishly shaving down from the 50MB wheels I have built and uploaded to S3 for CI.

@dopplershift dopplershift merged commit 92ba3c5 into matplotlib:master Jan 7, 2017
@QuLogic
Copy link
Member

QuLogic commented Jan 8, 2017

So is this getting backported?

@tacaswell
Copy link
Member

Yes, it also needs some docs, I will take care of it.

tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request Jan 10, 2017
By default, don't include tests in binary distributions.
@matthew-brett
Copy link
Contributor

Hmm - it makes testing the wheels quite a bit more complicated too - see https://travis-ci.org/MacPython/matplotlib-wheels/jobs/192055053#L1757 .

@dopplershift
Copy link
Contributor

Yes, but given that we're talking about removing 80% of the download bandwidth usage for matplotlib, I think we're saving almost a terabyte per month for PyPI with this (40MB * 10000/month = 400GB/month; yes, I'm making up the download number)

Adding the ability to point to the set of images somewhere else (maybe having a separate package for them) is probably the right answer. Of course, this is trivially done right now with pytest-mpl....

@matthew-brett
Copy link
Contributor

matthew-brett commented Jan 15, 2017

Yes, agreeing with what you said, we need a simple way of running the tests on a matplotlib install. At the moment what I'm having to do is:

  • build the mpl wheel with the tests enabled;
  • remove the baseline images from the built wheel;
  • install the wheel;
  • copy the baseline images from the source distribution into the installed location
    in order to run the tests.

See: https://github.com/MacPython/matplotlib-wheels/blob/master/config.sh

@dopplershift
Copy link
Contributor

dopplershift commented Jan 15, 2017

Would it be any easier/better to just install the wheel and run the tests out of a git checkout (provided the tests pick up the installed matplotlib)?

@matthew-brett
Copy link
Contributor

Sure - if I could guarantee that the code being executed was always the installed code, that would be fine, but I find the testing frameworks intimidating to analyze for this kind of thing. I also have to make sure the source and the wheel exactly correspond, but that's not an issue for the wheel building procedure.

In any case, it's reassuring to be able to run the tests on an installed distribution. For example, if someone's having trouble with an install, it would be useful for them to run the tests on their install, rather than having to run the tests on another copy they have cloned and built.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants