Adds release workflow #4112
Adds release workflow #4112
Conversation
|
Painful commit history! The release process looks like a huge improvement over our past one! |
It really was 😭 😭 😭 |
|
I've confirmed that the fixes to the docs workflow actually do the trick: https://github.com/epwalsh/allennlp/runs/606384562 |
|
And this confirms that the logging test has been fixed: https://github.com/epwalsh/allennlp/runs/606506610 |
| @@ -0,0 +1,32 @@ | |||
| # This Dockerfile creates an environment suitable for downstream usage of AllenNLP. | |||
| # It creates an environment that includes a wheel installation of allennlp. | |||
There was a problem hiding this comment.
Should we install -models into this too?
There was a problem hiding this comment.
Another option would be to publish a separate image that includes models. Could do that in the release workflow in the models repo
|
It's not great that it says "skipped" for all those tests, but not critical either. |
| name: CI | ||
|
|
||
| on: | ||
| # TODO: add nightly schedule. |
There was a problem hiding this comment.
So no nightly release right now?
There was a problem hiding this comment.
Yea, although I could probably get that done by the end of the day
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| python: ['3.6', '3.7'] |
There was a problem hiding this comment.
I know you just moved things from files, but maybe we should make the tests run on 3.6-8? As these are the 3 major Python versions we support.
There was a problem hiding this comment.
I'm not sure if we should support 3.8 yet. Last time I tried to use 3.8 I ran into a bunch of issues with dependencies not supporting it yet. That was a couple months ago so maybe those issues are fixed by now. In any case it would be worth trying but should probably hold off until we get the release process completely ironed out.
There was a problem hiding this comment.
@bryant1410, did you try it with 3.8? Does it work?
There was a problem hiding this comment.
I have a project with Python 3.8 with AllenNLP from master. It runs fine (unless there's a bug I'm not aware of...).
| run: | | ||
| make typecheck | ||
|
|
||
| - name: Run tests |
There was a problem hiding this comment.
What about also adding a python setup.py check? This performs checks package-wise, especially on the metadata.
There was a problem hiding this comment.
That's probably a good idea. It looks like it will fail though when we have a -unreleased version?
There was a problem hiding this comment.
Yeah, it breaks pip and PyPI. There are some rules of what are valid suffixes.
|
|
||
| - name: Install requirements | ||
| run: | | ||
| pip install --upgrade pip setuptools wheel |
There was a problem hiding this comment.
Is it necessary to specify "setuptools" and "wheel", given that they're here and will be installed if missing by the next pip install -e .?:
Line 22 in 0638cbb
There was a problem hiding this comment.
I did run into an issue once where some deps failed to install because wheel hadn't been installed yet. Maybe I was doing something weird at the time.
There was a problem hiding this comment.
I understand what you say. However, it should be installed now if missing because of that line I'm mentioning. See https://pip.pypa.io/en/stable/reference/pip/#pep-517-and-518-support
There was a problem hiding this comment.
Gotcha, I didn't know about that until now so thank you. It's probably a good idea to keep them up-to-date though. Do you know if pip will automatically do that?
There was a problem hiding this comment.
It won't, at least when I had problems with older Python versions. I'm not sure if it's necessary to keep them up-to-date but probably is necessary to set a minimum version. I had to set a minimum version for setuptools once, to support my features, because I saw the default in some configuration (Python 3.5?) wasn't working fine: pln-fing-udelar/fast-krippendorff@e8ec06f#diff-90829e76e906f1c73140c7ded7e1b268
| pip install -e . | ||
| pip install --upgrade -r dev-requirements.txt |
There was a problem hiding this comment.
It can be faster to do:
pip install --upgrade -e . -r dev-requirements.txtAs it resolves the dependencies only once.
There was a problem hiding this comment.
That's neat. It also gives it a chance to do a better job resolving dependencies.
There was a problem hiding this comment.
@bryant1410 feel free to make a PR for this
|
Oh, my comments were 5m late. |
Addresses #4096 and fixes other issues with the GitHub Actions workflows including the ssh problems with the docs deployment job. This also fixes the issue with the logging test.
Changes
The PR checks remain the same. What's different is what happens on a release or master commit. When either of these events occur, 4 more jobs will be ran once the usual PR checks are successful:
Build Package: Builds the PyPI distribution files.Test Package: Tests installing the core library from the distribution files.Docker: Builds, tests, and pushes a Docker image by installing the core library from the distribution files. This only runs once theTest Packagejob has completed successfully. On a master commit, the image will be pushed asallennnlp/commit:COMMIT_SHA, and on a release it will be pushed asallennlp/allennlp:RELEASE_TAG.Docs: Builds and pushes the documentation to theallennlp-docsrepo. This only runs once theTest Packagejob has completed successfully. On a master commit, the docs are put in themaster/folder, and on a release they are put into a folder with the name of the release tag.Further, for a release (not for a master commit), one more job named
PyPIruns after all of the other jobs mentioned complete successfully. This just uploads the distribution files to PyPI.Release process
With this updated workflow, the release process will look like this:
What this PR doesn't address