⬆️ Update to aiida-core v1.4.2 #48

chrisjsewell · 2020-10-13T21:57:09Z

Also drop separate numpy install, since this should no longer be needed, in accordance with: aiidateam/aiida-core#4378

Also drop sperate numpy install , since this should no longer be needed, in accordance with: aiidateam/aiida-core#4378

chrisjsewell · 2020-10-13T22:00:09Z

@ltalirz do you know if there was a reasoning behind this setuptools pinning ~=36.2:

ansible-role-aiida/tasks/aiida-prepare.yml

Line 78 in c709088

- setuptools~=36.2

given that aiida-core's build dependency is setuptools>=40.8.0,<50: https://github.com/aiidateam/aiida-core/blob/bd6903d88a4d88077150763574784a1bc375c644/pyproject.toml#L2

ltalirz · 2020-10-13T23:17:25Z

@ltalirz do you know if there was a reasoning behind this setuptools pinning ~=36.2:

@chrisjsewell I saw that it was introduced by me and I can't remember a specific reason that would still be valid. I think it's safe to delete.

ltalirz · 2020-10-13T23:20:25Z

This failure with numpy 1.19.2 seems weird... the "test-install" worfklow on the aiida-core repo is still working fine: https://github.com/aiidateam/aiida-core/runs/1245377894?check_suite_focus=true#step:4:61

chrisjsewell · 2020-10-13T23:23:12Z

As usual, it pymatgen being a pile ***. It looks like it requires cython to build, so will try adding cython to the virtulenv.

perhaps cython gets pre-installed on GH actions? 🤷

chrisjsewell · 2020-10-14T01:33:18Z

Yeh that looks to have fixed it.

One thought I had, before realising the issue with pymatgen, is that perhaps it would be better to use the CI requirements files, to use specific versions of dependencies (rather than them possibly changing over time), i.e.

- name: Install aiida-core dependencies
  pip:
    requirements: "{{ aiida_source_folder }}/aiida-core/requirements/requirements-py-3.{{ ansible_python_version.split('.')[1] }}.txt"
    virtualenv: "{{ aiida_venv }}"
    # According to https://github.com/ansible/ansible/issues/52275
    virtualenv_command: /usr/bin/python3 -m venv

- name: Install aiida-core package and dependencies
  pip:
    name:
    - "{{ aiida_source_folder }}/aiida-core"
    virtualenv: "{{ aiida_venv }}"
    # According to https://github.com/ansible/ansible/issues/52275
    virtualenv_command: /usr/bin/python3 -m venv
    editable: true
    extra_args: "--no-deps"
  notify: reentry scan
  # this guard is necessary because, for some reason, installs with
  # 'editable: true' always show as "changed"
  when: git_checkout.changed

The drawback would be that you could no longer specify particular extras,
although this feature is not actually utilized when building quantum-mobile.

Thoughts @ltalirz?

chrisjsewell · 2020-10-14T02:18:27Z

Hmm, I added a pip check final task. As you can see a few are failing (presumably they both have python 3.6) with: "notebook 5.7.10 has requirement nbconvert<6.0, but you have nbconvert 6.0.7."

ltalirz · 2020-10-14T08:39:23Z

perhaps it would be better to use the CI requirements files, to use specific versions of dependencies (rather than them possibly changing over time)

These would be the [tests] extra, right? Which translates to tests + rest + atomic_tools
https://github.com/aiidateam/aiida-core/blob/bd6903d88a4d88077150763574784a1bc375c644/setup.py#L30

Currently, the role defaults to even more extras:

ansible-role-aiida/defaults/main.yml

Lines 3 to 8 in c709088

    
           aiida_extras: 
        
           - rest 
        
           - docs 
        
           - atomic_tools 
        
           - testing 
        
           - notebook

It seems to me that being able to select extras is useful and perhaps we should actually reduce the number of extras installed by default to rest + atomic_tools to save some space. jupyter stuff will anyhow come with the aiidalab role if desired (perhaps removing notebook will solve the pip check issue) + I don't think users will typically want to build the documentation or run tests. We could then use the variable to install the "tests" extra on CI, which would allow us to run AiiDA tests if we wanted to.

The installation of AiiDA with extras from scratch is tested on the test-install workflow for python 3.6-3.8
https://github.com/aiidateam/aiida-core/blob/bd6903d88a4d88077150763574784a1bc375c644/.github/workflows/test-install.yml#L158
so I'm a bit puzzled as to why it was failing here... as you mention, perhaps cython is preinstalled in the CI environment or perhaps the platform is somehow recognized to match a pymatgen wheel?
I'm therefore also not sure whether switching to the fixed versions of requirements would have solved this problem.

Finally, would it make sense to add your pip check to the test-install workflow on aiida-core?
https://github.com/aiidateam/aiida-core/blob/develop/.github/workflows/test-install.yml
Or is this already handled a priori by using the 2020-resolver? (and should we use it here as well?)

ltalirz · 2020-10-14T08:47:16Z

tasks/aiida-prepare.yml

    - wheel
-    - numpy==1.17.4
+    - cython  # required by pymatgen


So, to recap: I think the problem is that pymatgen still specifies its build dependencies (numpy) via an antiquated method, which results in easy_install being used instead of pip, which then requires numpy to be built from source (rather than just using a wheel).
I.e. just putting numpy here should probably also solve the problem and would be faster.

Ok I'll give it a go thanks:
pymatgen does have numpy as a setup dependency not cython: https://github.com/materialsproject/pymatgen/blob/cea7587e40cff1d8b476601176b034d753301971/setup.py#L108

although feels weird that this is the dependency to build the cython files: https://github.com/materialsproject/pymatgen/blob/cea7587e40cff1d8b476601176b034d753301971/setup.py#L164

chrisjsewell · 2020-10-14T16:06:12Z

Finally, would it make sense to add your pip check to the test-install workflow on aiida-core?

I think it wouldn't hurt

cc @csadorf, FYI see above for some issues we've had; firstly with pymatgen install failing, then with pip check noting a notebook package version incompatibility

tasks/aiida-prepare.yml

chrisjsewell · 2020-10-16T10:42:40Z

Ermm what the heck, @ltalirz molecule has suddenly broken 🤦 (maybe a new version release, I'll check)

Run molecule test
ERROR: Failed to pre-validate.

{'driver': [{'name': ['unallowed value docker']}]}

latest versions:

 Collecting ansible~=2.9
  Downloading ansible-2.10.1.tar.gz (25.9 MB)
Collecting docker
  Downloading docker-4.3.1-py2.py3-none-any.whl (145 kB)
Collecting molecule~=3.0
  Downloading molecule-3.1.2-py3-none-any.whl (235 kB)

last working:

Collecting ansible~=2.9
  Downloading ansible-2.10.1.tar.gz (25.9 MB)
Collecting docker
  Downloading docker-4.3.1-py2.py3-none-any.whl (145 kB)
Collecting molecule~=3.0
  Downloading molecule-3.0.8-py3-none-any.whl (279 kB)

chrisjsewell · 2020-10-16T10:49:01Z

They moved the docker driver out of molecule core: ansible/molecule#2891 (comment)
That's not exactly a minor release is it!

ltalirz · 2020-10-16T10:49:15Z

Shame on them!

ltalirz · 2020-10-16T11:01:52Z

Poor geerlingguy did 87 commits in 87 repositories yesterday https://github.com/geerlingguy

ltalirz · 2020-10-16T11:04:47Z

Not very funny for him probably...

Perhaps he has an automated procedure for this? We may need to do the same.

They're not done via PRs, i.e. it's probably just a bash script iterating over all local repo folders.

chrisjsewell · 2020-10-16T11:07:35Z

Or using the github API?

chrisjsewell · 2020-10-16T11:16:21Z

Anyway I blame it all on @webknjaz 😝 (ansible/molecule#2891): ansible giveth and ansible taketh away

webknjaz · 2020-10-16T11:50:06Z

@chrisjsewell not my fault! I stopped maintaining molecule a long time ago. Blame @ssbarnea :)

espenfl · 2020-10-16T12:01:34Z

Poor geerlingguy did 87 commits in 87 repositories yesterday https://github.com/geerlingguy

Wow, that is indeed rather extensive. I suspect he does it from Ansible.

ltalirz · 2020-10-17T16:47:05Z

Ok, so this is what they suggest: https://github.com/openstack/tripleo-quickstart-extras/blob/a3cec1fd88f249b6997593f3e4d933c2f516d1cf/molecule-requirements.txt#L10
(one could also use molecule~=3.0.0).

@chrisjsewell Up to you whether you want to make the upgrade to 3.1 or just keep 3.0
This might be the only change needed for the transition: geerlingguy/ansible-role-helm@c8abca6

In the end, the fastest might be just to loop over local clones of all role repositories, commit and push.

chrisjsewell · 2020-10-21T03:33:10Z

TBH its OK, because I'm going through all repos and applying marvel-nccr/quantum-mobile#132 anyway

chrisjsewell · 2020-10-21T03:38:31Z

@ltalirz in a175318 I've solved the pip versioning issue 😄 (as long as the tests pass 🤞):

I now generate a pip constraints file from aiida-core's setup.json, then that is applied to all plugin installs, e.g. pip install -c constraints.txt aiida-plugin. This ensures no plugin can "override" the core version dependencies

chrisjsewell · 2020-10-21T03:47:08Z

ah damn this worked locally 🤔

chrisjsewell · 2020-10-21T04:34:05Z

haha @pradyun was just reading up on pip constraints and saw you snuck in tabs 👀

chrisjsewell · 2020-10-21T05:08:09Z

success!

ltalirz · 2020-10-21T09:01:25Z

defaults/main.yml

 - notebook
 aiida_data_folder: "${HOME}/.local/share/aiida"
 aiida_templates_folder: "${HOME}/.local/share/aiida"
 aiida_source_folder: "${HOME}/src"
 aiida_examples_folder: "${HOME}"
 aiida_venv: "${HOME}/.virtualenvs/aiida"
+aiida_constraints_file: "{{ aiida_data_folder }}/constraints.txt"
+# these are additional to those extracted from aiida-core and its extras
+aiida_constraints:


Should this be removed?

Ah, I see; we need to lock things down even further (?).
Should we be adding this constraint in the setup.json?

perhaps 🤷 (to the docs extra), I guess with the new pip --use-feature=2020-resolver it should resolve this and not allow such a broken environment to transpire. But without it, and with this particular set of extras it definitely creates a broken environment

@chrisjsewell that's why I advocate for having proper env requirements+pins managed by pip-tools instead of extras

Yeh we do already have lock files for CI testing: https://github.com/aiidateam/aiida-core/tree/develop/requirements.
The problem is that we are not just trying to install aiida-core, we need to install a range of other plugin packages that might not be exactly compatible with these hard pinnings:

ansible-role-aiida/defaults/main.yml

Lines 60 to 71 in c709088

aiida_plugin_versions:

aiida_cp2k: "1.1.0"

aiida_fleur: "1.1.0"

aiida_bigdft: "0.2.1a2"

# aiida_gudhi: "0.1.0a3"

# aiida_phtools: "0.1.0a1"

aiida_quantumespresso: "3.1.0"

# aiida_raspa: "1.0.0a2"

aiida_siesta: "1.1.0"

aiida_yambo: "1.1.1"

aiida_wannier90: "2.0.1"

aiida_wannier90_workflows: "1.0.1"

Oh, so you need a combined lockfile, I guess... Just dump that to a file and use a few requirements sources to produce transitive pins? FWIW I think what confused me is the definition substitution (constraints vs requirements).

Yeh exactly, I'm not completely sold on the current solution here, but at least now it is properly tested and produces a valid environment, which is a step forward 😬
So I am going to merge this as this, but anyway we have an open issue marvel-nccr/quantum-mobile#137 to consider this further, at which point I will definitely take into account your recommendation (thanks 🙏) and may well switch to some form of lock file

FYI @webknjaz you might be interested to see I now set up a full ~2 hour vagrant+ansible build running in GH Actions: https://github.com/marvel-nccr/quantum-mobile/runs/1286287172?check_suite_focus=true 😄

ltalirz · 2020-10-21T09:02:49Z

files/aiida-core-constraints.py

@@ -0,0 +1,37 @@
+#!/usr/bin/env python


Fine to keep it this parser here for the moment - eventually this should probably go to into https://github.com/aiidateam/aiida-core/tree/develop/utils

ltalirz · 2020-10-21T09:03:53Z

molecule/default/verify.yml

@@ -20,6 +20,16 @@
      - item.name in aiida_pps.stdout
    with_items: "{{ aiida_pseudopotentials }}"

+  - name: print aiida-core version
+    shell: "{{ aiida_venv }}/bin/pip show aiida-core"
+    changed_when: false


I think you don't even need this in verify.yml (of course fine to keep)

I thought just in case a plugin tries to install a different version

Sorry, I meant the changed_when is not needed (it's fine to change things in verify.yml)

ltalirz · 2020-10-21T09:05:15Z

tasks/aiida-prepare.yml

    - wheel
-    - numpy==1.17.4
+    - numpy  # required by pymatgen


no pinning, are you sure? ;-)

I'd feel safer with pinning it to some recent minor version

It is only needed for the pymatgen install, it will be updated, if necessary, by the pip install of aiida-core. If you pin it, then you have to ensure it is compatible with aiidalab and aiida-core (a current issue in the full quantum mobile build)

ltalirz

Great, thanks a lot @chrisjsewell

Just a few minor comments left, then we can merge this!

webknjaz · 2020-10-21T09:34:08Z

files/aiida-core-constraints.py

+import sys
+
+def remove_extra(req):
+    """Extras are not allowed in constraints files, e.g. dep[extra]>0.1 -> dep>0.1 """


why is that?

see here https://pip.pypa.io/en/latest/user_guide/#changes-to-the-pip-dependency-resolver-in-20-2-2020, which links to pypa/pip#6628

Ah, fair enough. Using pip-tools would fix that too.

webknjaz · 2020-10-21T09:35:19Z

files/aiida-core-constraints.py

@@ -0,0 +1,37 @@
+#!/usr/bin/env python
+"""Script to convert aiida setup.json to pip constraints file.


It's not constraints but rather requirements. You'd usually use https://github.com/jazzband/pip-tools to generate the pinned env deps aka constraints.

Also, pip-tools can generate pins from setup.py natively so you could just replace this script with an existing tool: https://github.com/jazzband/pip-tools#requirements-from-setuppy

no that creates a lock file, with strict version pinning. we just want a constraints file, for when you run subsequent pip installs, i.e. these installs can change the version of packages if necessary, but only within the ranges specified by setup.json

@chrisjsewell true but the purpose of constraints file in pip is exactly that it should be a lock file. So that the usage would be pip install -r requirements.txt -c constraints.txt. The former should usually contain open-ended direct dependencies while the latter would pinpoint what exactly is allowed (it's best when there's also --require-hashes too), including the transitive deps too, making the virtualenv setup reproducible.

ltalirz · 2020-10-21T12:40:43Z

@chrisjsewell If it simplifies the constraints issue, the installation of the plugins could probably be done in one go (it would be good to use a mechanism that allows specifying a commit/tag from a github repo as a source of a package instead of a pypi version; at least historically that was often needed).

The per-plugin yaml files could then be reduced to just setting up the corresponding aiida code.

chrisjsewell · 2020-10-21T12:48:16Z

@chrisjsewell If it simplifies the constraints issue, the installation of the plugins could probably be done in one go (it would be good to use a mechanism that allows specifying a commit/tag from a github repo as a source of a package instead of a pypi version; at least historically that was often needed).

The per-plugin yaml files could then be reduced to just setting up the corresponding aiida code.

Yeh as mentioned above I think I will merge this as is now, since it is definitely a step in the right direction. Then consider this further in marvel-nccr/quantum-mobile#137 (including how we integrate with aiidalab)

⬆️ Update to aiida-core v1.4.2

9950c1c

Also drop sperate numpy install , since this should no longer be needed, in accordance with: aiidateam/aiida-core#4378

chrisjsewell added 3 commits October 14, 2020 03:11

add cython to virtualenv

4a97fee

update plugin versions

02b92da

fix version

9e3c1d9

chrisjsewell added 3 commits October 14, 2020 03:54

unpin setuptools

a4d68f2

move plugin versions to separate release notes section

3c28f9e

fix pip check

d8e73a7

ltalirz reviewed Oct 14, 2020

View reviewed changes

chrisjsewell commented Oct 16, 2020

View reviewed changes

tasks/aiida-prepare.yml Outdated Show resolved Hide resolved

try installing numpy not cython

9344a4c

chrisjsewell added 2 commits October 20, 2020 17:25

Update rependencies

bd0463d

Add generation/use of pip constraints file

a175318

chrisjsewell requested a review from ltalirz October 21, 2020 03:39

chrisjsewell mentioned this pull request Oct 21, 2020

Address virtualenv compatibility issues (pip check) marvel-nccr/quantum-mobile#137

Open

chrisjsewell added 2 commits October 21, 2020 06:02

Add additional contraints variable

b4f5db3

add constraints to actual aiida-core install

e1c1f44

chrisjsewell added 3 commits October 21, 2020 06:36

comment out actual aiida-core

02e7f33

Update aiida-core-constraints.py

9262cf0

Update aiida-core-constraints.py

f8e7d97

chrisjsewell added 2 commits October 21, 2020 07:12

Update verify.yml

85639d9

Update verify.yml

1b933d1

ltalirz reviewed Oct 21, 2020

View reviewed changes

webknjaz reviewed Oct 21, 2020

View reviewed changes

chrisjsewell merged commit 3c89fc0 into master Oct 21, 2020

chrisjsewell deleted the aiida-core/v1.4.2 branch October 21, 2020 13:05

	aiida_plugin_versions:
	aiida_cp2k: "1.1.0"
	aiida_fleur: "1.1.0"
	aiida_bigdft: "0.2.1a2"
	# aiida_gudhi: "0.1.0a3"
	# aiida_phtools: "0.1.0a1"
	aiida_quantumespresso: "3.1.0"
	# aiida_raspa: "1.0.0a2"
	aiida_siesta: "1.1.0"
	aiida_yambo: "1.1.1"
	aiida_wannier90: "2.0.1"
	aiida_wannier90_workflows: "1.0.1"

		@@ -0,0 +1,37 @@
		#!/usr/bin/env python
		"""Script to convert aiida setup.json to pip constraints file.

⬆️ Update to aiida-core v1.4.2 #48

⬆️ Update to aiida-core v1.4.2 #48

Conversation

chrisjsewell commented Oct 13, 2020

chrisjsewell commented Oct 13, 2020

ltalirz commented Oct 13, 2020

ltalirz commented Oct 13, 2020

chrisjsewell commented Oct 13, 2020

chrisjsewell commented Oct 14, 2020 • edited Loading

chrisjsewell commented Oct 14, 2020

ltalirz commented Oct 14, 2020 • edited Loading

ltalirz Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell commented Oct 14, 2020

chrisjsewell commented Oct 16, 2020 • edited Loading

chrisjsewell commented Oct 16, 2020

ltalirz commented Oct 16, 2020

ltalirz commented Oct 16, 2020

ltalirz commented Oct 16, 2020 • edited Loading

chrisjsewell commented Oct 16, 2020

chrisjsewell commented Oct 16, 2020

webknjaz commented Oct 16, 2020

espenfl commented Oct 16, 2020

ltalirz commented Oct 17, 2020

chrisjsewell commented Oct 21, 2020

chrisjsewell commented Oct 21, 2020

chrisjsewell commented Oct 21, 2020

chrisjsewell commented Oct 21, 2020

chrisjsewell commented Oct 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltalirz Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltalirz Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltalirz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

webknjaz Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

ltalirz commented Oct 21, 2020 • edited Loading

chrisjsewell commented Oct 21, 2020

chrisjsewell commented Oct 14, 2020 •

edited

Loading

ltalirz commented Oct 14, 2020 •

edited

Loading

ltalirz Oct 14, 2020 •

edited

Loading

chrisjsewell commented Oct 16, 2020 •

edited

Loading

ltalirz commented Oct 16, 2020 •

edited

Loading

chrisjsewell Oct 21, 2020 •

edited

Loading

ltalirz Oct 21, 2020 •

edited

Loading

ltalirz Oct 21, 2020 •

edited

Loading

chrisjsewell Oct 21, 2020 •

edited

Loading

webknjaz Oct 21, 2020 •

edited

Loading

ltalirz commented Oct 21, 2020 •

edited

Loading