marvel-nccr / ansible-role-aiida

An ansible role that installs and configures AiiDA on Ubuntu.
Other
2 stars 5 forks source link

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

Closed chrisjsewell closed 3 years ago

chrisjsewell commented 3 years ago

Also drop separate numpy install, since this should no longer be needed, in accordance with: https://github.com/aiidateam/aiida-core/pull/4378

chrisjsewell commented 3 years ago

@ltalirz do you know if there was a reasoning behind this setuptools pinning ~=36.2: https://github.com/marvel-nccr/ansible-role-aiida/blob/c709088dff74d1e1ae4d8379e740aba35fb2ef97/tasks/aiida-prepare.yml#L78

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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: https://github.com/marvel-nccr/ansible-role-aiida/blob/c709088dff74d1e1ae4d8379e740aba35fb2ef97/defaults/main.yml#L3-L8

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?)

chrisjsewell commented 3 years ago

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

chrisjsewell commented 3 years ago

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 commented 3 years ago

They moved the docker driver out of molecule core: https://github.com/ansible-community/molecule/issues/2891#issuecomment-709956623 That's not exactly a minor release is it!

ltalirz commented 3 years ago

Shame on them!

ltalirz commented 3 years ago

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

ltalirz commented 3 years ago

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 commented 3 years ago

Or using the github API?

chrisjsewell commented 3 years ago

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

webknjaz commented 3 years ago

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

espenfl commented 3 years ago

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 commented 3 years ago

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: https://github.com/geerlingguy/ansible-role-helm/commit/c8abca698c01783cc2cff694747e98252bb66c15

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

chrisjsewell commented 3 years ago

TBH its OK, because I'm going through all repos and applying https://github.com/marvel-nccr/quantum-mobile/issues/132 anyway

chrisjsewell commented 3 years ago

@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 commented 3 years ago

ah damn this worked locally 🤔

chrisjsewell commented 3 years ago

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

chrisjsewell commented 3 years ago

success!

ltalirz commented 3 years ago

@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 commented 3 years ago

@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 https://github.com/marvel-nccr/quantum-mobile/issues/137 (including how we integrate with aiidalab)