Closed chrisjsewell closed 4 years ago
Thanks a lot @chrisjsewell - since this is quite a bit of stuff to review, I suggest that only one of marco and me does this. Since Marco needs to be up to date with the current build process it might be useful for him to do the review - @borellim let me know if this is a problem, then I can find the time to do it.
I've read through the list of changes in the PR and they all seem to go into a good direction - just for the addition of tox it is not clear to me whether this really makes things easier but I guess I'll see once I try it out.
In any case, once the changes are converged, I'm happy to try a full Quantum Mobile build myself using this PR before it is merged.
just for the addition of tox it is not clear to me whether this really makes things easier
trust me, tox makes everything easier 😉
Yeh no problem. FYI I would still like to get auth access to the openstack account, so I can upload myself
Got the website up and running: https://quantum-mobile.readthedocs.io/en/gh-actions/ (still needs some work)
Got the website up and running: quantum-mobile.readthedocs.io/en/gh-actions (still needs some work)
the documentation is basically finished, if you want to take a skim @ltalirz
Thanks for the review @ltalirz!
I think I've covered all your comments now
As discussed previously, I'm still not convinced of including release information in the docs and I suggest to change this back
We'll agree to disagree on this 😬. I think it's a horrible user experience, to tell them they have to go find releases on GitHub. Yes make releases, but then defer the release notes to the repository, where the information can be properly maintained and version controlled. This is what I do in all my repositories, for example: https://github.com/executablebooks/MyST-Parser/releases, and it what many other prominent repositories do, such as: https://github.com/jupyterlab/jupyterlab/blob/master/docs/source/getting_started/changelog.rst It is also discouraged by: https://keepachangelog.com/en/1.0.0/#frequently-asked-questions
Don't forget to put redirect notices from the wiki in place since, again, the FAQs are linked from the Desktop of the QM (as well as from other places).
Yep, I intended to remove the content of the wiki and put links to the documentation.
Also marvel-nccr.quantum_mobile_customizations/templates/faq.desktop
should be altered to point to the documentation.
Thanks Chris, I've gone through the comments and resolved most of them (+ replied in some).
As for the release notes, how about this: let's indeed keep the detailed changelog in the docs (there was anyhow a changelog.md in the repository before - just checking that this information is not lost?), and link to it from the github releases page but let's keep the installation instructions and contact info (which anyhow don't fit into a changelog) in the release notes.
The changelog in the docs could in principle be just on a single page (easier to scan/search) but if you prefer individual posts you can of course do that as well.
I.e. the release notes on github would be
Get Quantum Mobile running on your computer in three simple steps:
quantum_mobile_20.06.1.ova
MD5 hash: dba6be9e60e5adf1618a6e251e579c1b
Login credentials: username: max
, password: moritz
.
The default configuration of 2
cores and 1536
MB RAM can be adjusted in the VM settings.
For issues encountered during installation, please first consult the FAQ page.
Please direct inquiries regarding Quantum Mobile to the AiiDA mailinglist
See the [Quantum Mobile documentation]().
One question: since we're anyhow using tox to create the environment, is there a need to support python 3.6-3.8?
$ tox -a -v
using tox.ini: /Users/leopold/Personal/Postdoc-MARVEL/repos/quantum-mobile/tox.ini (pid 23987)
using tox-3.20.1 from /Users/leopold/Applications/miniconda3/envs/qmobile/lib/python2.7/site-packages/tox/__init__.pyc (pid 23987)
default environments:
py38-vagrant -> Build a vagrant box VM then provision with ansible
additional environments:
py37-vagrant -> Build a vagrant box VM then provision with ansible
py36-vagrant -> Build a vagrant box VM then provision with ansible
py36-ansible -> Run ansible on an existing VM
py38-package -> Package a built Vagrant box for distribution
docs-clean -> Build the documentation
py37-ansible -> Run ansible on an existing VM
py37-package -> Package a built Vagrant box for distribution
py36-package -> Package a built Vagrant box for distribution
docs-update -> Build the documentation
py38-ansible -> Run ansible on an existing VM
docs-live -> Build the documentation and launch browser
Is there any downside to just offer python 3.8 (in which case one can also drop the py.. prefix)?
Edit: oh ok, now I see that tox can only use interpreters it already sees... So people actually need to first figure out their python version before they know which tox command they can use... hm
Edit: oh ok, now I see that tox can only use interpreters it already sees... So people actually need to first figure out their python version before they know which tox command they can use... hm
Yeh I was meaning to look into what the best way to handle this was for some other packages.
docs-update
works fine, so I guess if you don't specify a py3X
it just chooses whats available. But then obviously you lose the ability to specify what python version you want to use (I have all three available via tox-conda
). But for this package I don't think it should be an issue to run with any python.
So I think just remove the py3X-
prefixes
and add basepython = python3
to the tox.ini: https://tox.readthedocs.io/en/latest/config.html#conf-basepython
@chrisjsewell I've run the commands and it seems to have worked just fine :-) (I also didn't test the VM; but that's for later)
I've pinged you in two open comments from the first review; apart from that there are just the suggestions from above and we should be good to go.
@ltalirz in 31547ff I improved the tox (and updated the documentation) so now you just get:
$ tox -a -v
using tox.ini: /Users/chrisjsewell/Documents/GitHub/quantum-mobile/tox.ini (pid 83422)
using tox-3.20.0 from /Users/chrisjsewell/opt/miniconda3/lib/python3.7/site-packages/tox/__init__.py (pid 83422)
default environments:
vagrant -> Build a vagrant box VM then provision with ansible
additional environments:
docs-update -> Build the documentation (modify any existing build)
docs-clean -> Build the documentation (remove any existing build)
ansible -> Run ansible on an existing VM
package -> Package a built Vagrant box for distribution
docs-live -> Build the documentation and launch browser
BTW have you tried tox -e docs-live
?
As you modify the documentation, it automatically triggers rebuilds and reloads the browser, which I think is pretty neat 😄
BTW have you tried tox -e docs-live?
I just tried but for me building the watchdog
wheel failed
Command "/Users/leopold/Personal/Postdoc-MARVEL/repos/quantum-mobile-tox/.tox/docs-live/bin/python -u -c "import setuptools, tokenize;__file__='/private/var/folders/8y/7c7qkxcx5vxb749rt3sw5sh40000gp/T/pip-install-n0l20bmr/watchdog/setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, __file__, 'exec'))" install --record /private/var/folders/8y/7c7qkxcx5vxb749rt3sw5sh40000gp/T/pip-record-emrih3p5/install-record.txt --single-version-externally-managed --compile --install-headers /Users/leopold/Personal/Postdoc-MARVEL/repos/quantum-mobile-tox/.tox/docs-live/include/site/python3.6/watchdog" failed with error code 1 in /private/var/folders/8y/7c7qkxcx5vxb749rt3sw5sh40000gp/T/pip-install-n0l20bmr/watchdog/
Great, thanks for all the work, from my side this is good to squash & merge :-)
thanks!
I just made one last change in fec5d9f, to improve the auto-generated release notes, if you want to look
I just tried but for me building the watchdog wheel failed
oh that's annoying
Also initiated one last test build: https://github.com/marvel-nccr/quantum-mobile/runs/1258369990
I just made one last change in fec5d9f, to improve the auto-generated release notes, if you want to look
Thanks, looks good and useful!
Also initiated one last test build: https://github.com/marvel-nccr/quantum-mobile/runs/1258369990
Ok! Feel free to merge afterwards; I'll then add this minor update
Let's get this PR merged, and then proceed with the inclusion of bigdft and the actual release
Head-lines:
.github/workflows/build.yml
now runs the full vagrant box build, clean, compacting and image creation.This PR re-works the build process to be more streamlined:
yaml_parser.sh
,compact_hd.sh
andcreate_image.sh
scripts have been consolidated intoplaybook-package.yml
. This process writes the image and release notes into adist/
folder.playbook.yml
->playbook-build.yml
hosts
andglobalconfig.yml
have been consolidated intoinventory.yml
, and the readglobalconfig.yml
tasks have been removed. This contains all the global variables, and also host specific variables; effectively deprecating thequantum-mobile-cloud
repository. To run against a specific host, simply callansible-playbook playbook-build.yml --extra-vars "build_hosts=aws"
Vagrantfile
, the ansible provisioning now runs against a static IP, which is specified ininventory.yml
. This allows for ansible to use all the variables defined ininventory.yml
, rather than working from a separate auto-generated inventory, ensuring consistency..github/workflows/ci.yml
tox -e py38-vagrant
(installs all python/ansible role dependencies then runsvagrant up
), ortox -e py38-ansible
, when running ansible against an existing VM, ortox -e py38-package
to run the VM compacting and image creation tasks..github/workflows/build.yml
; this runs the full vagrant box build and packaging to completion via GH actions. Obviously this won't be run for every PR commit (as it is currently set up to do for testing), but could be run for each commit to master or something like that. Ideally, this would actually upload the build to somewhere, then we could just manually download/test it before releasing. I was trying to get it to save as an artifact, but currently there is an issue with this (see https://github.com/actions/upload-artifact/issues/29#issuecomment-706512345), we could probably set it up to upload straight to openstack though, or even a google drive account (https://github.com/marketplace/actions/upload-files-to-google-drive). The two current limitations are that 1) you can only set the build VM to have 1 CPU 2) At least with the base installed software on the GH VM, setting-accelerate3d
toon
fails the VM start up.Still TODO:
Other notes (for seperate PRs):
release_notes
task to aiidalab, specifying its versionaiida_venv
andaiidalab_venv
should probably be mergedaiida
role, write plugin package versions to separate release_notes sectionresources/Release.md.j2
(add build versions: Virtual Mobile, vagrant, ansible)@ltalirz @borellim, since it's just the documentation left to do, I would encourage you to give a review.