Cleanup dependencies #3597

sphuber · 2019-12-03T12:58:50Z

Fixes #3547 and fixes #2903 and fixes #3528

This PR releases the strict pinning of dependency versions.
In addition, it gets rid of some unnecessary dependencies:

mock
passlib
uritools

giovannipizzi · 2019-12-03T20:00:28Z

setup.json

+      "pymatgen>=2019.7.2",
+      "pymysql~=0.9.3",
+      "seekpath~=1.9",
+      "spglib~=1.14"
    ],
    "notebook": [
      "jupyter==1.0.0",


I think these requirements were to avoid collisions with py2 and maybe now we can just put jupyter and notebook, without version specification?

No unfortunately we can't. It's true that notebook>=6 does not support python 2, but it also requires pyzmq>=17 and tornado>=5 and we, because of circus (and plumpy and kiwipy) have exactly pyzmq<17 and tornado<5. I had made a start into seeing whether I could fix circus and make it compatible with the newer versions and made some headway but it is not trivial. Hopefully I can look into this more with @muhrin during the coding week

giovannipizzi · 2019-12-03T20:01:18Z

setup.json

    ],
    "bpython": [
-      "bpython==0.17.1"
+      "bpython~=0.18.0"


Do we want to drop this? Is any user really using this? I guess it's more for convenience of users, but then maybe it's ok to remove?

I am not sure if anyone uses it, so that's why I left it in for now.

giovannipizzi

Seems reasonable to me! Thanks

sphuber · 2019-12-03T21:16:50Z

@asle85 and @waychal I have removed quite a few dependencies in the rest extras. The tests are running so apparently they weren't used internally, at least not in the tested paths. Can you please check that this makes sense?

ltalirz

thanks @sphuber for all this work - and great that we can upgrade to django 2!

I just have some minor suggestions, which is to not downgrade any of the dependencies we currently have (unless you have tested with the older versions).

I'll now also give installing this a try on mac.

P.S. Sorry for having made the edits in reqs_for_rtd - I realized too late.

docs/requirements_for_rtd.txt

setup.json

ltalirz · 2019-12-04T11:22:21Z

install on macos seems fine

csadorf · 2019-12-04T11:25:09Z

Hi, I have not reviewed all changes to the dependencies in detail yet, but I have had prepared a draft for a related AEP that I just had not submitted yet. Maybe we can consider this in the context of this PR?

sphuber · 2019-12-04T11:26:59Z

@ltalirz thanks for the review. The changes you proposed are not necessary. The changes I made will not result in adowngrade. The compatible operator ~= will install the latest version compatible with the spec. For example, for ete3 the latest is version 3.1.1. When doing pip install ete3~=3.1 it will install 3.1.1. This is the whole reason I adopted this specifier and for dependencies with a major release >=1 we will only ever specify the minor version. In this way the latest minor version will always be installed. See the explanation in the first commit for more details.

Edit: I performed a test in a clean virtualenv to make sure:

(aiida_test) sphuber@theos:~/code/aiida/env/dev$ pip install ete3~=3.1 --no-cache-dir
Collecting ete3~=3.1
  Downloading https://files.pythonhosted.org/packages/21/17/3c49b7fafe10ed63bb7904ebf9764b98db726aa5fd482fb006818854bc04/ete3-3.1.1.tar.gz (10.5MB)
     |████████████████████████████████| 10.5MB 3.4MB/s 
Building wheels for collected packages: ete3
  Building wheel for ete3 (setup.py) ... done
  Created wheel for ete3: filename=ete3-3.1.1-cp36-none-any.whl size=2226265 sha256=f3c6348dd907d1219faae0381f7ae4f477042d5a3b4829c5dbd29e5ec8e2db46
  Stored in directory: /tmp/pip-ephem-wheel-cache-u850vplo/wheels/14/90/50/5c10d965e7fb68343d9c5349cce2787ff3e38397946468d640
Successfully built ete3
Installing collected packages: ete3
Successfully installed ete3-3.1.1

ltalirz · 2019-12-04T11:43:11Z

@ltalirz thanks for the review. The changes you proposed are not necessary. The changes I made will not result in adowngrade. The compatible operator ~= will install the latest version compatible with the spec.

I should have been a bit more specific - it is true that installing aiida would not result in downgrading of packages, but if you install AiIDA in an environment that already has an older version of the package installed, then it will not upgrade.
And we have not tested AiiDA with the older version of the package

$ pip install wrapt==1.11.1
Collecting wrapt==1.11.1
  Using cached https://files.pythonhosted.org/packages/67/b2/0f71ca90b0ade7fad27e3d20327c996c6252a2ffe88f50a95bba7434eda9/wrapt-1.11.1.tar.gz
Building wheels for collected packages: wrapt
  Building wheel for wrapt (setup.py) ... done
  Stored in directory: /Users/leopold/Library/Caches/pip/wheels/89/67/41/63cbf0f6ac0a6156588b9587be4db5565f8c6d8ccef98202fc
Successfully built wrapt
aiida-core 1.0.1 has requirement django~=2.2, but you'll have django 1.11.25 which is incompatible.
aiida-core 1.0.1 has requirement numpy~=1.17, but you'll have numpy 1.16.4 which is incompatible.
aiida-optimade 0.10.0 has requirement fastapi[all]==0.28.0, but you'll have fastapi 0.42.0 which is incompatible.
aiida-optimade 0.10.0 has requirement lark-parser==0.5.6, but you'll have lark-parser 0.7.8 which is incompatible.
Installing collected packages: wrapt
  Found existing installation: wrapt 1.11.2
    Uninstalling wrapt-1.11.2:
      Successfully uninstalled wrapt-1.11.2
Successfully installed wrapt-1.11.1
$ pip install wrapt~=1.11
Requirement already satisfied: wrapt~=1.11 in /Users/leopold/Applications/miniconda3/envs/aiida_rmq_py3/lib/python3.7/site-packages (1.11.1)

sphuber · 2019-12-04T11:54:07Z

I see, but by specifying the patch version, we strictly reduce the accepted range of versions to that specific minor version, a limitation the removal of which was the whole point of this PR. So there is a direct conflict between those two requirements. We could just tell people in the installation documentation that when installing in an existing environment, the should run pip with the --upgrade flag. This will do the right thing. And for new environments there is no problem.

elsapassaro · 2019-12-04T12:03:25Z

Both I and @waychal have checked the dependencies removed in the rest extras and it looks fine to us. Given that the tests are passing it's ok to remove them.

ltalirz · 2019-12-04T12:12:08Z

I see, but by specifying the patch version, we strictly reduce the accepted range of versions to that specific minor version, a limitation the removal of which was the whole point of this PR.

That is not how the ~= operator works:

$ pip install wrapt~=1.11.1
Collecting wrapt~=1.11.1
aiida-core 1.0.1 requires reentry~=1.3, which is not installed.
Installing collected packages: wrapt
  Found existing installation: wrapt 1.10.11
    Uninstalling wrapt-1.10.11:
      Successfully uninstalled wrapt-1.10.11
Successfully installed wrapt-1.11.2

Even if you specify a patch version, it will install the latest compatible version.
It's a clever operator ;-)

sphuber · 2019-12-04T12:23:31Z

That is not how the ~= operator works:

I am afraid it does. Consider the following two examples:

(aiida_test) sphuber@theos:~/code/aiida/env/dev$ pip install numpy~=1.16.1 --no-cache-dir
Collecting numpy~=1.16.1
  Downloading https://files.pythonhosted.org/packages/98/87/41283370f942f647422581eed16df4b653a744a3e9d5cfbb9aee0440f6eb/numpy-1.16.5-cp36-cp36m-manylinux1_x86_64.whl (17.4MB)
     |████████████████████████████████| 17.4MB 3.3MB/s 
Installing collected packages: numpy
Successfully installed numpy-1.16.5

versus

(aiida_test) sphuber@theos:~/code/aiida/env/dev$ pip install numpy~=1.16 --no-cache-dir
Collecting numpy~=1.16
  Downloading https://files.pythonhosted.org/packages/d2/ab/43e678759326f728de861edbef34b8e2ad1b1490505f20e0d1f0716c3bf4/numpy-1.17.4-cp36-cp36m-manylinux1_x86_64.whl (20.0MB)
     |████████████████████████████████| 20.0MB 3.5MB/s 
Installing collected packages: numpy
Successfully installed numpy-1.17.4

When you specify a patch version, it will always remain within the corresponding minor version, as shown by the first example. Doing numpy~=1.16.1 will install numpy==1.16.5 as that is the last patch of the 1.16 minor series. Doing numpy~=1.16 (without explicit patch version) instead will install numpy==1.17.4. It is the latter that we want, which is why I removed the explicit patch versions for dependencies with a major version >=1.

ltalirz · 2019-12-04T13:06:08Z

You're right - and apologies for not reading your commit message/comments properly (I didn't realize we are only fixing the major version).

I think we should retain the ability to force upgrades of the patch level of certain packages, if we know that they contain important bug fixes (such as pgtest 1.3.1).
Otherwise, at some point we will have cases where users will upgrade their AiiDA instance and be stuck with an old, broken dependency.

pip install --upgrade would indeed solve the problem... but will people remember it?
And I think it will upgrade all dependencies of dependencies as well, which may be something the user does not want.

How about something like pgtest~=1.3,>=1.3.1? That would work, no?

sphuber · 2019-12-04T13:14:41Z

pip install --upgrade would indeed solve the problem... but will people remember it?
And I think it will upgrade all dependencies of dependencies as well, which may be something the user does not want.

Probably not in some cases, but that is the case with many things. I agree that we should try to make things as robust as possible but this is not always possible in the case of competing and directly conflicting goals.

How about something like pgtest~=1.3,>=1.3.1? That would work, no?

I think this does indeed work and would be a possible solution and certainly makes a lot of sense for dependencies where we no a particular patch version contains important bug fixes. I don't think we should blindly apply this to all our current versions though. For the majority of dependencies, the specific version we have currently were just picked because they were the latest version that was available the last time I updated all the dependencies en masse. Do you know of any other dependency that might have a specifically required patch version?

ltalirz · 2019-12-04T13:18:40Z

Do you know of any other dependency that might have a specifically required patch version?

pgtest is the only one I know for sure - as a "gut feeling" I would also add sqlalchemy (very large patch version), seekpath (probably want the latest version here), prospector (also here we want the latest version), psycopg2-binary (not sure, would just be bad if we have connection issues because of some old version)

P.S. Of course, these extra >= specifications can be dropped in the future, whenever one updates the ~= to a later version.

greschd · 2019-12-04T13:33:50Z

setup.json

+      "pg8000~=1.13",
+      "pgtest~=1.3",
+      "pytest~=5.3",
+      "sqlalchemy-diff~=0.1.3"
    ],
    "dev_precommit": [


I think for the pre-commit dependencies it actually makes sense to pin them exactly. The issue with these is that even a "backwards compatible" change in behavior can cause the build to fail.

For example, when pylint has a false positive, we add a # pylint: disable=... comment. If a subsequent patch fixes the false positive, it will trigger useless-suppression errors.

Also, they are presumably used only when developing aiida-core - so there really isn't much of a concern for being compatible with other environments / packages.

We have the useless-suppression warning disabled globally, but I see your point. This can also just happen with pre-commit and yapf that introduce new formatting rules. However, if that happens, since these are only for developers anyway, the dev just upgrades his dependencies in his development environment and commits the changes no?

Just imagine your PR fails because in just that moment some new yapf/pylint version has been released...
I would prefer to avoid this ;-)

Exactly, it can be quite an annoying thing. Also, if we have scheduled CI on older branches they will just suddenly fail with pre-commit errors, for no reason.

sphuber · 2019-12-04T13:47:23Z

@ltalirz I have added explicit lower required patch versions for psycopg-binary, seekpath, sqlalchemy and pgtest. Only question then remains what we do with the pre-commit extras. Do we pin those to the version that we had for now?

ltalirz · 2019-12-04T13:49:05Z

Do we pin those to the version that we had for now?

Yes, I think @greschd has a good point here.

sphuber · 2019-12-04T13:57:16Z

@greschd and @ltalirz ok I pinned the packages of dev_precommit to an explicit version, with the exception of toml. Since that one is not used for actual linting or formatting, I think there we still automatically want updates in patch versions.

ltalirz

thanks @sphuber , both for the cleanup and for your patience ;-)

sphuber · 2019-12-04T14:22:56Z

Seems like a little bit more patience is required 😅 . The pre-commit step now fails because for some reason astroid=2.3.3 is installed but prospector==1.1.7 requires 2.2.5. Maybe because I relaxed the requirements before it installed 2.3.3 and now it is using the cache?

ltalirz · 2019-12-04T14:35:14Z

That's very strange... With your previous version of the branch (all dependencies with ~=) that I have locally), it is installing astroid 2.2.5.

The only main difference I can see is that you were using pre-commit 1.19.

Perhaps 1.17 had some astroid dependency that pip is somehow not properly resolving?

P.S. Here the output of pip freeze from your previous version of the branch
freeze.log

sphuber · 2019-12-04T14:38:53Z

Perhaps 1.17 had some astroid dependency that pip is somehow not properly resolving?

I know that prospector demands an explicit version, see their setup.py. What I don't understand who required astroid==2.3.3 and why prospectors requirement does not get respected during setup.

sphuber · 2019-12-04T14:49:45Z

Seems like it was a problem with the cache. I added a commit to run pip install without cache and now it properly installed astroid==2.2.5.

The version requirements of all `install_requires` and `extras_require` dependencies are reevaluated. Where we used to require explicit version by pinning to exact versions using the `==` operator, we now use the more liberal operator `~=`. This is the compatible release operator: https://www.python.org/dev/peps/pep-0440/#compatible-release For a given release identifier `V.N` the compatible release clause is approximately equivalent to the pair of comparison clauses: >= V.N, == V.* With this rule we define our dependency requirements using the minor version for packages with a major version of 1 or higher and using the patch version for packages whose major version is still at 0. The reason is that when packages have released `v1` they can be expected to respect semantic versioning and not break backwards compatibility in minor version whereas this is typically not the case for `v0` versions where backward incompatible changes are introduced in minor versions as well. Note that this approach will not prevent from breaking our builds because packages break their API in minor versions, but at least we have a consistent dependency requirement policy. List of specific restrictions and changes: * `pymatgen==2019.7.2` is the last to support python 3.5 * `jinja2` moved to `install_requires` as it is used explicitly by `verdi` * `astroid==2.2.5` required by `prospector`, will cause crash if wrong Explicit lower patch requirements are set for a few packages because they are known to contain critical and required bug fixes: * `psycopg-binary>=2.8.3` * `sqlalchemy>=1.3.10` * `pgtest>=1.3.1` * `seekpath>=1.9.3` Finally, an exception is made for packages of the `dev_precommit` extra which can affect pre-commit behavior such as linting warnings or code formatting. To prevent updates in these packages from unexpectedly failing the pre-commit hooks, they are pinned to an explicit version.

This is a rolling backport of the module `unittest.mock` which has been part of the standard python library since v3.3. Note that one has to do `from unittest.mock import patch` and use it directly as for some reason `import unittest` followed by using `unittest.mock.patch` will result in an `AttributeError`.

It was only used to parse the repository URI of a profile, which can be accomplished just as well with the built in `urllib.parse.urlparse`.

They will be removed in `aiida-core==2.0.0` which will allow to drop the `ete3` dependency. This CLI command was they only code using this lib.

sphuber force-pushed the fix_3547_dependency_requirements branch from 6f8cd63 to 1dc29a4 Compare December 3, 2019 18:44

sphuber changed the title ~~[BLOCKED] Cleanup dependencies~~ Cleanup dependencies Dec 3, 2019

sphuber requested review from ltalirz, giovannipizzi and csadorf December 3, 2019 18:45

giovannipizzi reviewed Dec 3, 2019

View reviewed changes

giovannipizzi previously approved these changes Dec 3, 2019

View reviewed changes

ltalirz suggested changes Dec 4, 2019

View reviewed changes

sphuber mentioned this pull request Dec 4, 2019

Dependency management aiidateam/AEP#10

Merged

5 tasks

greschd reviewed Dec 4, 2019

View reviewed changes

sphuber dismissed giovannipizzi’s stale review via dd78c06 December 4, 2019 13:46

sphuber force-pushed the fix_3547_dependency_requirements branch from 1dc29a4 to dd78c06 Compare December 4, 2019 13:46

sphuber force-pushed the fix_3547_dependency_requirements branch from dd78c06 to 98f35aa Compare December 4, 2019 13:56

ltalirz approved these changes Dec 4, 2019

View reviewed changes

sphuber force-pushed the fix_3547_dependency_requirements branch 3 times, most recently from a3f4502 to f18369c Compare December 4, 2019 17:12

sphuber added 6 commits December 4, 2019 18:29

Remove unnecessary explicit dependency uritools

69096b9

It was only used to parse the repository URI of a profile, which can be accomplished just as well with the built in `urllib.parse.urlparse`.

Remove unused explicit dependency passlib

ed11e58

Deprecate verdi node tree and related utilities

1a07adc

They will be removed in `aiida-core==2.0.0` which will allow to drop the `ete3` dependency. This CLI command was they only code using this lib.

Update dependency requirement for Django to 2.2

8885d21

sphuber force-pushed the fix_3547_dependency_requirements branch from f18369c to 8885d21 Compare December 4, 2019 17:30

sphuber merged commit c7455a0 into aiidateam:develop Dec 4, 2019

sphuber deleted the fix_3547_dependency_requirements branch December 4, 2019 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup dependencies #3597

Cleanup dependencies #3597

sphuber commented Dec 3, 2019 •

edited

Loading

giovannipizzi Dec 3, 2019

sphuber Dec 3, 2019

giovannipizzi Dec 3, 2019

sphuber Dec 3, 2019

giovannipizzi left a comment

sphuber commented Dec 3, 2019

ltalirz left a comment •

edited

Loading

ltalirz commented Dec 4, 2019

csadorf commented Dec 4, 2019

sphuber commented Dec 4, 2019 •

edited

Loading

ltalirz commented Dec 4, 2019 •

edited

Loading

sphuber commented Dec 4, 2019

elsapassaro commented Dec 4, 2019

ltalirz commented Dec 4, 2019 •

edited

Loading

sphuber commented Dec 4, 2019 •

edited

Loading

ltalirz commented Dec 4, 2019

sphuber commented Dec 4, 2019

ltalirz commented Dec 4, 2019 •

edited

Loading

greschd Dec 4, 2019

greschd Dec 4, 2019 •

edited

Loading

sphuber Dec 4, 2019

ltalirz Dec 4, 2019

greschd Dec 4, 2019

sphuber commented Dec 4, 2019

ltalirz commented Dec 4, 2019

sphuber commented Dec 4, 2019

ltalirz left a comment

sphuber commented Dec 4, 2019

ltalirz commented Dec 4, 2019 •

edited

Loading

sphuber commented Dec 4, 2019

sphuber commented Dec 4, 2019

Cleanup dependencies #3597

Cleanup dependencies #3597

Conversation

sphuber commented Dec 3, 2019 • 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

giovannipizzi left a comment

Choose a reason for hiding this comment

sphuber commented Dec 3, 2019

ltalirz left a comment • edited Loading

Choose a reason for hiding this comment

ltalirz commented Dec 4, 2019

csadorf commented Dec 4, 2019

sphuber commented Dec 4, 2019 • edited Loading

ltalirz commented Dec 4, 2019 • edited Loading

sphuber commented Dec 4, 2019

elsapassaro commented Dec 4, 2019

ltalirz commented Dec 4, 2019 • edited Loading

sphuber commented Dec 4, 2019 • edited Loading

ltalirz commented Dec 4, 2019

sphuber commented Dec 4, 2019

ltalirz commented Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

greschd Dec 4, 2019 • 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

sphuber commented Dec 4, 2019

ltalirz commented Dec 4, 2019

sphuber commented Dec 4, 2019

ltalirz left a comment

Choose a reason for hiding this comment

sphuber commented Dec 4, 2019

ltalirz commented Dec 4, 2019 • edited Loading

sphuber commented Dec 4, 2019

sphuber commented Dec 4, 2019

sphuber commented Dec 3, 2019 •

edited

Loading

ltalirz left a comment •

edited

Loading

sphuber commented Dec 4, 2019 •

edited

Loading

ltalirz commented Dec 4, 2019 •

edited

Loading

ltalirz commented Dec 4, 2019 •

edited

Loading

sphuber commented Dec 4, 2019 •

edited

Loading

ltalirz commented Dec 4, 2019 •

edited

Loading

greschd Dec 4, 2019 •

edited

Loading

ltalirz commented Dec 4, 2019 •

edited

Loading