Skip to content
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

Streamline releasing and testing #202

Merged
merged 33 commits into from
Jan 9, 2018
Merged

Streamline releasing and testing #202

merged 33 commits into from
Jan 9, 2018

Conversation

ceball
Copy link
Member

@ceball ceball commented Oct 26, 2017

This is a PR about streamlining param releases (fixes #167), and
improving testing.

  1. Added conda recipe and package upload

    Builds and uploads conda packages for (non-PR/simulated) commits to master. I.e. latest master is always available as a conda package from anaconda.org/cball/param.

  2. Added pypi package building and uploading

    For tags, builds sdist and universal wheel, and uploads to test.pypi (simulating actual releases to pypi).

  3. Enabled testing on Windows via Appveyor.

    Param is pure python, but it's useful to test on windows to catch accidental platform specific content (whether that's code, developer workflow, testing, or whatever). Fixes Test on appveyor #195.

  4. Use tox for testing

    There is now very little scripting in the travis and appveyor config files (except for conda, which has no integration with travis). The same tests can be run locally as on CI. Nothing is platform specific.

    Tox not only tests 'python setup.py develop', but also creates an sdist, installs it, and tests it. Fixes Travis should test setup.py and resulting package #192.

    Changes include:

    • Moved dependencies out of platform-specific travis file and into setup.py (if genuine dependency) or tox (if test dependency).

    • Now tests that optional dependencies are actually optional. Previously, many other packages were present in the test environment. Have also started to test that specific functionality works when the optional dependencies are present. Fixes Travis: test different environments #189.

    • Fixed running of flake8.

    • Warnings during tests are now displayed (fixes nosetests should display warnings #86).

    Being python only, param can take advantage of the standard python ecosystem, so unfortunately not all of this approach is suitable for conda-dependent packages.

Also, minor updates to README, including coverage badge (not sure why hiding it makes sense; closes #65).

and improving testing.

(1) Added conda recipe and package upload

    Builds and uploads conda packages for (non-PR/simulated) commits
    to master.  I.e. latest master is always available as a conda
    package from anaconda.org/cball/param.

(2) Added pypi package building and uploading

    For tags, builds sdist and universal wheel, and uploads to
    test.pypi (simulating actual releases to pypi).

(3) Enabled testing on Windows via Appveyor.

    Param is pure python, but it's useful to test on windows to catch
    accidental platform specific content (whether that's code,
    developer workflow, testing, or whatever). Fixes #195.

(4) Use tox for testing

    There is now very little scripting in the travis and appveyor
    config files (except for conda, which has no integration with
    travis). The same tests can be run locally as on CI. Nothing is
    platform specific.

    Tox not only tests 'python setup.py develop', but also creates an
    sdist, installs it, and tests it. Fixes #192.

    Changes include:

      * Moved dependencies out of platform-specific travis file and
        into setup.py (if genuine dependency) or tox (if test
        dependency).

      * Now tests that optional dependencies are actually
        optional. Previously, many other packages were present in the
        test environment. Have also started to test that specific
        functionality works when the optional dependencies are
        present. Fixes #189.

      * Fixed running of flake8.

      * Warnings during tests are now displayed (fixes #86).

    Being python only, param can take advantage of the standard python
    ecosystem, so unfortunately not all of this approach is suitable
    for conda-dependent packages.

Also, minor updates to README, inclusing coverage badge (not sure why
hiding it makes sense; closes #65).
@ceball
Copy link
Member Author

ceball commented Oct 26, 2017

Still got two things to address (noted with TODOs in setup.py, by commented-out code).

Can't build or test sdist between releases

sdist is the basis of a lot of packaging, and those packages are the things that actually get installed, so it makes sense to try creating, installing, and testing the sdist. However, can't currently do that:

$ git status
On branch pkg
Your branch is ahead of 'origin/pkg' by 1 commit.
  (use "git push" to publish your local commits)
nothing to commit, working tree clean
$ git describe --dirty
v1.5.1-29-g400ab17
$ python setup.py sdist
Traceback (most recent call last):
  File "setup.py", line 45, in <module>
    param.__version__.verify(setup_args['version'])
  File "/Users/cball/code/ioam/param6/param/version.py", line 316, in verify
    raise Exception("Supplied string version does not match current version.")
Exception: Supplied string version does not match current version.

"Latest master" conda packages will not have useful version info

Although the packages themselves will have a reasonable version (see ioam/parambokeh#15), the installed param package will have a version of just e.g. 1.5.1.

@ceball ceball requested a review from jbednar October 26, 2017 23:17
Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me, and I look forward to having this all running smoothly. I'm not sure about the status of your two TODO items; looks like one can only be tested once on master (in which case pushing commits to master directly is ok by me), but I'm not sure about the other one.

- TOXENV: py33
- TOXENV: py27
- TOXENV: py26

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't suppose anyone has written a command for Appveyor that will import the relevant bits from .travis.yml to avoid duplication?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, sigh. At least it's overall a pretty short file now.

I think the ideal thing would be if appveyor and travis supported tox, then you'd only have the tox file. One problem on travis is that the 'build stages' I use here were added to travis fairly recently (they are still beta), so not even all of travis's own stuff seems to work with them yet. Otherwise I could maybe have used something like https://tox-travis.readthedocs.io/en/stable/.

I have used tox before on another project in the past (so it's been around a bit), and liked it. What I mainly like is that:

  • the config is short...
  • ...yet it covers a lot (particularly the testing of packages)
  • and the config is separate from your code, i.e. does not tie itself in to your code. So (a) it is not a big deal to replace if something else comes along, and (b) I can use tox locally, but e.g. you don't have to if you don't want (you can just run one or more of the commands from tox.ini manually if you want, like nosetests).

It would be great if there were something providing similar functionality in the conda world, and if there were a conda plugin for travis, etc :)

.travis.yml Outdated
- <<: *tests
python: 3.5
env: TOX_ENV=py35

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a shame that this part had to get so verbose, but it seems worth it.

e.g. ``tox -e coverage`` to run unit tests with coverage for the
currently active python. Alternatively, unit tests can be run via
``nosetests`` (after installing `nose
<http://nose.readthedocs.io/en/latest/>`_).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you don't want to switch from nose to pytest?

Copy link
Member Author

@ceball ceball Oct 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I probably do want to - if only to match the bulk of other projects I encounter right now, not because I understand the difference ;)

I guess I was planning to switch at the same time as adding a unit test or two on parambokeh (just to get started there, not to try to test everything!!).

@ceball
Copy link
Member Author

ceball commented Oct 27, 2017

I'm not sure about the status of your two TODO items; looks like one can only be tested once on master

Right

but I'm not sure about the other one

This is the one I need to do something about (not just for this PR, but for parambokeh conda packages too). There are two parts to it.

First, I'd like the python package in the conda package to have a meaningful version. I'm considering something like this (proposed by Jean-Luc): python -m param.version --set-conda-info ., which could add __version__.commit_count = <N> and __version__.commit = <SHA> after the existing __version__ in __init__ (would need to change those being from read-only properties in param.version).

Second, I can't actually make sdist between tags at the moment, even to test them, without commenting out a version check. Jean-Luc has proposed modifying the commented-out guard in setup.py to if ('upload' in sys.argv) or ('sdist' in sys.argv) and not 'PARAM_DISABLE_VERSION_CHECK' in os.environ:, so I'm considering that. (E.g. I don't even know if 'upload' is a relevant argument or not any more for pypi uploading - need to check.)

setup.py Outdated
# TODO: conda packages won't have meaningful version number yet

# TODO: need to decide what to do about this
# if ('upload' in sys.argv) or ('sdist' in sys.argv):
Copy link
Contributor

@jlstevens jlstevens Oct 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also condition this on the presence of an environment variable when you need this check disabled.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what my earlier comment says ;) It's hard to follow because this same discussion was happening in parambokeh too. I'll try to keep it here, and I'll ping you to review when I actually try something. Thanks!

(I've never quite understood this line, because aside from sdist there's also various bdist args one might pass instead. And nowadays I'm not sure anyone would do 'python setup.py upload' - I will check that. So some of the delay is that I need to update myself on python packaging.)

@ceball
Copy link
Member Author

ceball commented Oct 29, 2017

@jlstevens, it would be great if you could comment on the behavior I describe below - thanks!

Recent hacks mean this can't currently be merged, but they show behavior that's getting closer to what I think I want. Basically, the idea is that every new PR merge (i.e. commit) to master will result in a new "dev" release being uploaded to anaconda.org automatically.

# e.g. just released 1.5.1, now starting work on 1.5.2
$ git commit -m "Bump version to 1.5.2dev0"
$ git tag -a v1.5.2dev0
$ git commit -m "Something"
$ git commit -m "Something else"
$ git describe --dirty
v1.5.2.dev0-2-g143ea80
$ PARAM_MAKING_UNOFFICIAL_RELEASE=1 python setup.py sdist
[...]
creating param-1.5.2.dev2+g143ea80
[...]
$ mv dist/param-1.5.2.dev2+g143ea80 /tmp/
$ pushd /tmp/
$ tar xf param-1.5.2.dev2+g143ea80.tar.gz 
$ cd param-1.5.2.dev2+g143ea80/
$ python
>>> import param
>>> param.__version__
1.5.2.dev2+g143ea80

Similarly, PARAM_MAKING_UNOFFICIAL_RELEASE=1 conda build conda.recipe/ gives me a conda package with the name param-1.5.2.dev2_g143ea80-pyh0d9cc94_0.tar.bz (pyh0d9cc94 is a conda build hash). Now when I do conda install -c (the dev channel) param and then I import param, I get a version of 1.5.2.dev2+g143ea80.

(I think conda supports pep440, so the conda package name could maybe be param-1.5.2.dev2+g143ea80-pyh0d9cc94_0.tar.bz (untested). But the underscore is what you get by default from conda build's GIT_BUILD_STR.)

@ceball
Copy link
Member Author

ceball commented Dec 15, 2017

@jlstevens and @jbednar: the recent commits to this PR leave it demonstrating (at least in my mind...) how versioneer allows the workflow described in holoviz-dev/autover#2. (Albeit I have not changed from the 'pre' versioning scheme I described before Jim suggested a better alternative.)

However: the >2000 lines of versioneer I'm sure revokes Jim's previous review approval ;)

I don't propose to merge this, but I would love to have all projects operating like this. conda packages of master are always available (see https://anaconda.org/cball/param/files - although here they are enabled for the branch, because I am testing), and 'release candidates' can be made any time with a git tag ... && git push --tags.

@ceball
Copy link
Member Author

ceball commented Dec 15, 2017

And holoviz-dev/autover#9 sets up the same workflow on autover itself (but not working), as a target.

@jbednar
Copy link
Member

jbednar commented Dec 15, 2017

Apart from the 2000 lines, that all sounds good to me. Hopefully Jean-Luc can get that down to 20.

@ceball
Copy link
Member Author

ceball commented Jan 5, 2018

This PR's been open for over 2 months now. @jbednar, I've removed the versioneer demo, so do you re-approve of merging?

@@ -0,0 +1,40 @@
"""
If numpy's present, is numpy stuff ok?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nice to have a better description here...

@jbednar
Copy link
Member

jbednar commented Jan 5, 2018

Yes, I re-approve of merging.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 74.937% when pulling 0a84f12 on packaging into 64c8f7c on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 74.937% when pulling 606a583 on packaging into 64c8f7c on master.

@ceball ceball merged commit 787732a into master Jan 9, 2018
@philippjfr philippjfr deleted the packaging branch September 27, 2018 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants