CI: fix problems in the code, pre-commit and docs-build workflows #5301

sphuber · 2022-01-12T15:49:43Z

First commit is not 100% necessary but related to the fix of pymatgen and it is time for the change anyway following AEP 003.
The other three commits solve the problem of each failing workflow.

According to AEP 003, which states that we follow NEP 029, support for NumPy 1.18 can be dropped as of December 22 2021.

The link used to provide more background information on the concept of exit codes has been experiencing downtime often recently, causing the builds to fail in the `docs-build` workflow.

ramirezfranciscof

Yeah, all looks good to me. I feel like I was not on the wrong track updating mypy and trying to solve the new problems, it is incredibly frustrating that my local mypy is misbehaving. Not being able to set a reliable work environment is hell 😞.

Anyways, thanks for the fix! Just had a couple of questions but probably ok to go ahead and merge this.

pyproject.toml

setup.json

ramirezfranciscof · 2022-01-12T16:07:33Z

BTW also pinging @csadorf for informing on the dependency updates.

chrisjsewell

well err there is a 2 line fix for the pre-commit issue: #5302. (although I guess we could update the mypy/sqlalchemy at some point anyway)
Whatever you do, I would suggest pinning the install of requirements, which will lead to less unexpected failures when a package releases a new version

chrisjsewell · 2022-01-12T16:13:30Z

the pymatgen fix is good though 👍

codecov · 2022-01-12T16:15:33Z

Codecov Report

Merging #5301 (ae592f1) into develop (c743a33) will increase coverage by 0.01%.
The diff coverage is 89.19%.

@@             Coverage Diff             @@
##           develop    #5301      +/-   ##
===========================================
+ Coverage    81.51%   81.51%   +0.01%     
===========================================
  Files          530      530              
  Lines        37103    37105       +2     
===========================================
+ Hits         30242    30244       +2     
  Misses        6861     6861

Flag	Coverage Δ
django	`76.98% <83.79%> (+0.01%)`	⬆️
sqlalchemy	`75.97% <89.19%> (-<0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/orm/nodes/process/calculation/calcjob.py	`78.00% <ø> (ø)`
aiida/engine/daemon/execmanager.py	`67.83% <20.00%> (-0.52%)`	⬇️
aiida/engine/processes/builder.py	`92.99% <100.00%> (ø)`
aiida/engine/processes/ports.py	`97.85% <100.00%> (ø)`
aiida/orm/implementation/entities.py	`92.31% <100.00%> (ø)`
aiida/orm/implementation/sqlalchemy/backend.py	`90.98% <100.00%> (ø)`
aiida/orm/nodes/node.py	`96.41% <100.00%> (ø)`
aiida/plugins/factories.py	`92.39% <100.00%> (ø)`
aiida/repository/common.py	`94.21% <100.00%> (ø)`
aiida/repository/repository.py	`96.54% <100.00%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c743a33...ae592f1. Read the comment docs.

sphuber · 2022-01-12T16:16:41Z

@chrisjsewell I updated the last commit to include your changes. Figured that even though that is the better fix, it is maybe still worth it to keep the update to mypy==0.930.

chrisjsewell · 2022-01-12T16:18:47Z

setup.json

+            "sqlalchemy[mypy]~=1.4.29",
            "sqlalchemy2-stubs"


sqlalchemy[mypy] is the same as sqlalchemy2-stubs: https://docs.sqlalchemy.org/en/14/orm/extensions/mypy.html#installation

so we remove the sqlalchemy2-stubs one?

Removed the stubs one. I have to leave now and can't respond again until later tonight

yeh heres he exact axtra: https://github.com/sqlalchemy/sqlalchemy/blob/a869dc8fe3cd579ed9bab665d215a6c3e3d8a4ca/setup.cfg#L45-L47,
well I feel the version of sqlalchemy itself should come from the main requirements lock file, so I would remove sqlalchemy[mypy]~=1.4.29

But then we won't be automatically updated if sqlachemy adds another requirement to the mypy extra. And only upping the version in our pre-commit extra makes sense since that more recent version is only required to make the typing work. It is not necessary for the normal install so would be a bit restrictive to unnecessarily up the minimum requirement there as well, wouldn't it?

Its just obviously possible for the sqlalchemy version to then get out of sync, and for you to end up "further away" from the requirements in the requirements.txt file. But honestly it's not a big thing either way, I'll leave it up to you

chrisjsewell · 2022-01-12T16:21:28Z

Figured that even though that is the better fix, it is maybe still worth it to keep the update to mypy==0.930.

yeh it can't hurt 👍

csadorf · 2022-01-13T09:04:11Z

.github/workflows/test-install.yml

+    - name: Build pymatgen with compatible numpy
+      run: |
+        # This step is necessary because certain versions of `pymatgen` will not specify an explicit version of
+        # `numpy` in its build requirements, and so the latest version will be used. This causes problems,
+        # however, because this means that the compiled version of `pymatgen` can only be used with that version
+        # of `numpy` or higher, since `numpy` only guarantees forward compatibility of the ABI. If we want to
+        # run with an older version of `numpy`, we need to ensure that `pymatgen` is built with that same
+        # version. This we can accomplish by installing the desired version of `numpy` manually and then calling
+        # the install command for `pymatgen` with the `--no-build-isolation` flag. This flag will ensure that
+        # build dependencies are ignored and won't be installed (preventing the most recent version of `numpy`
+        # to be installed) and the build relies on those requirements already being present in the environment.
+        # We also need to install `wheel` because otherwise the `pymatgen` build will fail because `bdist_wheel`
+        # will not be available.
+        pip install numpy==1.21.4 wheel
+        pip install pymatgen==2022.0.16 --no-cache-dir --no-build-isolation
+


Unless we also change our installation documentation, we cannot add this step here. The steps carried out as part of this workflow are particular designed to check that the steps documented as part of the user documentation for installing this package are working as expected. It seems like we will have to make that more obvious as part of the action's documentation.

Furthermore, afaik, the test-install flow has not failed due to the pymatgen issue, so there should be no need to "fix" it in this way either.

Fair enough, will revert this. That being said, the existing workflow explicitly updates pip and setuptools adding a comment that the latter is critical for an install of pymatgen. This is not reflected in the documentation. But maybe this is no longer applicable and should also be removed?

Indeed, maybe we add a comment to the docs that we test against the latest version of setuptools and pip and if there are any issues, users should try to update them? We could also explicitly test against the minimal version supported according to our build settings and the latest version?

My inclination would be to not make this more complicated until we run into problems.

Seems reasonable to me. But I would suggest we leave this for another PR as it seems beyond the scope of getting the CI back up and running.

setup.json

csadorf · 2022-01-13T09:16:23Z

setup.json

@@ -86,7 +86,7 @@
            "PyCifRW~=4.4",
            "ase~=3.18",
            "matplotlib~=3.3,>=3.3.4",
-            "pymatgen>=2019.7.2,<=2022.02.03,!=2019.9.7",
+            "pymatgen>=2019.7.2,!=2019.9.7",


Pymatgen does not follow semantic versioning, not sure whether it is a good idea to leave this unbounded. Any future release could break our package.

Added <=2022.1.9 which is the latest version and for which our tests pass.

With the release of `numpy==1.22.0` on December 31 2022, our CI builds started failing. The reason being that `pymatgen` would get built with a version of `numpy` that is incompatible with the version of `numpy` that would be installed in the environment in which the tests are run. The reason for this is that `pymatgen` does not specify an explicit version of `numpy` in its build requirements and so will pick the latest version. But `numpy` only guarantees foreward compatibility with its ABI and so if compiled with a certain version, the resulting binary won't be compatible with older `numpy` versions. In this case explicitly, `pymatgen` was getting built with `numpy==1.22` but then the tests were run with `1.21.4` which would cause the exception. The solution is to ensure `pymatgen` is compiled with a version of `numpy` that is ABI-compatible with the version that will be used in the actual test run. The way to do this is to first explicitly install a version of `numpy` that we need and then install `pymatgen` using the `--no-build-isolation` flag to force `pip` to not install build requirements but take them from the existing environment. This is also why we need to install the `wheel` package explicitly, because this is also a build requirement of `pymatgen` that no longer will be installed automatically by `pip` due to `--no-build-isolation`. Finally, there was a necessary change for the dependencies of Python 3.10 where we update the version of `pymatgen` because older versions could not be built since the building used CPython code that has been removed in Python 3.10.

The pre-commit step was failing due to an exception in `mypy`. This was caused by a bug in the typing of `sqlalchemy`. Upping the requirement in the `pre-commit` extra to `sqlalchemy[mypy]~=1.4.29` makes sure that we install a version that fixes the problem. The `mypy` extra will install the `sqlalchemy2-stubs` package, so that explicit requirement is removed. Note that these new versions require `mypy==0.930` so we update that version. The update in `mypy` comes with some improvements that allow us to remove some previous ignore statements and others have to be added or updated. In addition, we ensure that we install pinned versions in the workflow by installing from the requirements file, and then just adding the additional dependencies from the `pre-commit` extra.

ramirezfranciscof · 2022-01-13T09:48:44Z

In the end we might not need to fix this ourselves anymore! Or at least we will be able to revert it as soon as a new pymatgen releases: materialsproject/pymatgen@ca664f1

(we should still go ahead and merge this to fix the tests now, just saying for the near future)

chrisjsewell

ta

sphuber · 2022-01-13T10:43:35Z

In the end we might not need to fix this ourselves anymore! Or at least we will be able to revert it as soon as a new pymatgen releases: materialsproject/pymatgen@ca664f1

(we should still go ahead and merge this to fix the tests now, just saying for the near future)

Indeed, I asked them to fix this, but even if they release this, if we want to rely on this, we would have to up our minimum requirement to that latest release, which is maybe not something we want. But maybe we do, not sure to be honest.

csadorf · 2022-01-13T10:51:50Z

we would have to up our minimum requirement to that latest release, which is maybe not something we want. But maybe we do, not sure to be honest.

I would recommend to update this and maybe a few other major requirements with version 2.0.

sphuber added 2 commits January 12, 2022 13:04

Dependencies: drop support for NumPy 1.18

64b3dd2

According to AEP 003, which states that we follow NEP 029, support for NumPy 1.18 can be dropped as of December 22 2021.

Docs: replace link for exit code with more stable one

6e95d15

The link used to provide more background information on the concept of exit codes has been experiencing downtime often recently, causing the builds to fail in the `docs-build` workflow.

sphuber requested review from chrisjsewell, ramirezfranciscof and csadorf January 12, 2022 15:49

ramirezfranciscof previously approved these changes Jan 12, 2022

View reviewed changes

pyproject.toml Show resolved Hide resolved

setup.json Show resolved Hide resolved

chrisjsewell suggested changes Jan 12, 2022

View reviewed changes

sphuber dismissed ramirezfranciscof’s stale review via cbfbf26 January 12, 2022 16:15

sphuber force-pushed the fix/5293/dependencies-pymatgen-numpy branch from f933893 to cbfbf26 Compare January 12, 2022 16:15

sphuber requested a review from chrisjsewell January 12, 2022 16:16

chrisjsewell reviewed Jan 12, 2022

View reviewed changes

sphuber force-pushed the fix/5293/dependencies-pymatgen-numpy branch from cbfbf26 to c3decf4 Compare January 12, 2022 16:28

sphuber mentioned this pull request Jan 13, 2022

minimal fix for pre-commit issue #5302

Closed

csadorf suggested changes Jan 13, 2022

View reviewed changes

sphuber added 2 commits January 13, 2022 10:40

sphuber force-pushed the fix/5293/dependencies-pymatgen-numpy branch from c3decf4 to ae592f1 Compare January 13, 2022 09:41

sphuber requested review from chrisjsewell and csadorf January 13, 2022 09:41

csadorf approved these changes Jan 13, 2022

View reviewed changes

chrisjsewell approved these changes Jan 13, 2022

View reviewed changes

sphuber merged commit 995f254 into aiidateam:develop Jan 13, 2022

sphuber deleted the fix/5293/dependencies-pymatgen-numpy branch January 13, 2022 10:08

chrisjsewell mentioned this pull request Mar 12, 2022

🔀 MERGE: Release v2.0.0b1 #5426

Merged

sphuber mentioned this pull request Mar 13, 2022

Dependencies: Drop support for NumPy 1.17 #5095

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: fix problems in the code, pre-commit and docs-build workflows #5301

CI: fix problems in the code, pre-commit and docs-build workflows #5301

sphuber commented Jan 12, 2022

ramirezfranciscof left a comment

ramirezfranciscof commented Jan 12, 2022

chrisjsewell left a comment •

edited

Loading

chrisjsewell commented Jan 12, 2022

codecov bot commented Jan 12, 2022 •

edited

Loading

sphuber commented Jan 12, 2022

chrisjsewell Jan 12, 2022 •

edited

Loading

sphuber Jan 12, 2022

sphuber Jan 12, 2022

chrisjsewell Jan 12, 2022

sphuber Jan 13, 2022

chrisjsewell Jan 13, 2022

chrisjsewell commented Jan 12, 2022

csadorf Jan 13, 2022

sphuber Jan 13, 2022

csadorf Jan 13, 2022

sphuber Jan 13, 2022

csadorf Jan 13, 2022

sphuber Jan 13, 2022

ramirezfranciscof commented Jan 13, 2022 •

edited

Loading

chrisjsewell left a comment

sphuber commented Jan 13, 2022

csadorf commented Jan 13, 2022

CI: fix problems in the code, pre-commit and docs-build workflows #5301

CI: fix problems in the code, pre-commit and docs-build workflows #5301

Conversation

sphuber commented Jan 12, 2022

ramirezfranciscof left a comment

Choose a reason for hiding this comment

ramirezfranciscof commented Jan 12, 2022

chrisjsewell left a comment • edited Loading

Choose a reason for hiding this comment

chrisjsewell commented Jan 12, 2022

codecov bot commented Jan 12, 2022 • edited Loading

Codecov Report

sphuber commented Jan 12, 2022

chrisjsewell Jan 12, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell commented Jan 12, 2022

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

ramirezfranciscof commented Jan 13, 2022 • edited Loading

chrisjsewell left a comment

Choose a reason for hiding this comment

sphuber commented Jan 13, 2022

csadorf commented Jan 13, 2022

chrisjsewell left a comment •

edited

Loading

codecov bot commented Jan 12, 2022 •

edited

Loading

chrisjsewell Jan 12, 2022 •

edited

Loading

ramirezfranciscof commented Jan 13, 2022 •

edited

Loading