Migrate to aiida-core 2.0 #331

unkcpz · 2022-03-09T02:04:46Z

Update readme to have a aiida-core dep matrix, since it is not backward compatible. Will then have the major version changed.

codecov · 2022-03-09T02:29:36Z

Codecov Report

Merging #331 (e7255ed) into develop (cb1b59f) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #331      +/-   ##
===========================================
+ Coverage    93.03%   93.05%   +0.02%     
===========================================
  Files           32       32              
  Lines         1363     1353      -10     
===========================================
- Hits          1268     1259       -9     
+ Misses          95       94       -1

Flag	Coverage Δ
aiida	`89.43% <100.00%> (-0.01%)`	⬇️
mongo	`79.74% <42.85%> (+0.58%)`	⬆️

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

Impacted Files	Coverage Δ
aiida_optimade/cli/__init__.py	`100.00% <ø> (ø)`
aiida_optimade/mappers/structures.py	`89.47% <ø> (ø)`
aiida_optimade/routers/structures.py	`100.00% <ø> (ø)`
aiida_optimade/cli/cmd_calc.py	`83.87% <100.00%> (+0.26%)`	⬆️
aiida_optimade/cli/cmd_init.py	`99.20% <100.00%> (+<0.01%)`	⬆️
aiida_optimade/translators/entities.py	`86.30% <100.00%> (-0.45%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

CasperWA

You don't seem to be updating the AiiDA version in this PR?

.github/workflows/ci_tests.yml

CasperWA · 2022-03-09T10:17:36Z

Ah sorry - this was still a draft - never mind me :)

unkcpz · 2022-04-20T11:33:10Z

Hi @CasperWA, I see you add commits to the PR, thanks! Sorry that I am late to finish this clearly, I was distracted by other projects and just recovered from COVID.

There are two things in the TODO list for this migration to v2.0.0: 1. There are lots of deprecation warnings need to be take care which means use the old API that not being supported. 2. For the loglevel, I change INFO to REPORT which I think it is not proper. The better solution is to use logging for aiida-optimade rather that call click log which will using aiida-core loglevel setting.

unkcpz · 2022-04-20T15:50:09Z

@CasperWA I revert the echo_report to echo_info by explicitly setting aiida.cmdline log level to INFO for the command init and calc. The deprecation warnings are cleaned. The tests are all passed. It is ready for review.

eimrek · 2022-05-25T10:04:40Z

Hi both, I tested this a bit, and if I add a structure without calling aiida-optimade init, then i'm getting an 500 Internal server error, see:

After calling aiida-optimade -p quicksetup init, it seems to work well.

unkcpz · 2022-05-25T10:23:43Z

Hi @CasperWA, do you have more expectations on what I can add to this PR? Please let me know. I think @eimrek, basically can go ahead and test based on this branch on materials cloud.

CasperWA · 2022-05-25T11:08:23Z

After calling aiida-optimade -p quicksetup init, it seems to work well.

At this point in time, one indeed needs to initialize/prime (run aiida-optimade init) before a profile can be exposed via the OPTIMADE API. So I'm not surprised. But for ease-of-use this might be a feature to implement in the future: on-the-fly initialization support.

CasperWA

This is looking really good.

Please remove the .vscode folder (and maybe add it to your global .gitignore file, see e.g., this StackOverflow answer on how to set that up).

Dockerfile

aiida_optimade/cli/__init__.py

aiida_optimade/cli/cmd_init.py

aiida_optimade/translators/entities.py

requirements.txt

CasperWA · 2022-05-25T11:18:23Z

Let me just merge in ~~master~~develop properly here.

Edit: Done.

eimrek · 2022-05-25T13:47:06Z

After calling aiida-optimade -p quicksetup init, it seems to work well.

At this point in time, one indeed needs to initialize/prime (run aiida-optimade init) before a profile can be exposed via the OPTIMADE API. So I'm not surprised. But for ease-of-use this might be a feature to implement in the future: on-the-fly initialization support.

Hi Casper, just to be clear: it's happening even when first initialization has already been run, but afterwards when adding new structures this problem occurs. E.g. it contradicts the README sentence of

If in the future, more StructureData nodes are added to your profile's database, these will be automatically updated for the first query, filtering on any of these OPTIMADE-specific fields

However, i'm not sure if this problem was already there before or this PR introduced this (I currently don't have aiida < 2.0 to test quickly).

CasperWA · 2022-05-25T13:52:43Z

Hi Casper, just to be clear: it's happening even when first initialization has already been run, but afterwards when adding new structures this problem occurs. E.g. it contradicts the README sentence of

If in the future, more StructureData nodes are added to your profile's database, these will be automatically updated for the first query, filtering on any of these OPTIMADE-specific fields

However, i'm not sure if this problem was already there before or this PR introduced this (I currently don't have aiida < 2.0 to test quickly).

Right! Let's just create an issue for it then, and we/I can take a look at it.

Ah - just remembering now. It might be really important to inspect this part of the code and whether it should be updated. I needed to go directly through the specific backend to update node extras in bulk. This might have changed in the new version.

unkcpz

@CasperWA thanks for reviewing. I adopt most of your suggestions. And leave some open questions. Please have another look when you are avaliable.

unkcpz · 2022-05-25T21:43:34Z

aiida_optimade/translators/entities.py


            with get_manager().get_backend().transaction() as session:
                session.query(DbNode).filter(DbNode.id == self._pk).update(
                    values={"extras": extras}


Here could be the issue mentioned by @eimrek rooted. As I TODO comment above, I guess you did not load_node and call (re)set_extra from performance consideration? I think there may be a way to use sqla interface to do this without performance issue?

I'd like to @sphuber for a comment. The purpose of the code here was simply to update extras attributes. Is calling load_node(<pk>) then set_extras at the expense of efficiency? If so how to do it properly with sqla directly?

To be honest I am not sure anymore how much going to our front-end slows down these queries. It would be good to test since it has been refactored a lot and the slowdown may not be so bad anymore compared to before. If that is the case, I would definitely advise going through the front-end such that all storage backends are automatically supported, especially now that they are pluginnable with aiida v2.0.

That's fair enough, and some benchmarking would be more than welcome. I'm pretty sure this will come out to still be faster, as the Node is never (supposedly) retrieved from the database, but rather a specific DB row's extras column is updated.

aiida_optimade/cli/cmd_init.py

aiida_optimade/translators/entities.py

requirements.txt

unkcpz · 2022-05-31T15:10:19Z

Hi Casper, just to be clear: it's happening even when first initialization has already been run, but afterwards when adding new structures this problem occurs. E.g. it contradicts the README sentence of
If in the future, more StructureData nodes are added to your profile's database, these will be automatically updated for the first query, filtering on any of these OPTIMADE-specific fields
However, i'm not sure if this problem was already there before or this PR introduced this (I currently don't have aiida < 2.0 to test quickly).

Right! Let's just create an issue for it then, and we/I can take a look at it.

Ah - just remembering now. It might be really important to inspect this part of the code and whether it should be updated. I needed to go directly through the specific backend to update node extras in bulk. This might have changed in the new version.

I check with old version, it also has this problem. And I guess this is why we need aiida-optimade -p <profile> calc to add things to extras attributes for new added StructureData or CifData?

I open an issue from what @eimrek reported at #358

unkcpz · 2022-05-31T15:31:22Z

I have to remove support to python3.10 for the moment, since it rely on urllib3 and it not support python3.10 yet. I open an issue #359 to add it after urllib3=2.0.0 is released or we can move to other alternative libraries.

unkcpz · 2022-05-31T19:44:03Z

Screen.Recording.2022-05-31.at.21.32.08.mov

Screen.Recording.2022-05-31.at.21.35.45.mov

@sphuber @CasperWA Here are some benchmarks about direct load node and update extras with aiida API and using DbNode. The first record is in commit cd2cfa3 the DbNode way to query and update extras. The second one is the latest commit using node.set_extra directly. I think there is no significant performance difference, they are all about 5-10 items/second. I think it is pretty safe to make this change.

unkcpz · 2022-08-17T22:16:46Z

Also add the dep matrix as like aiida-quantumespresso, can check from https://github.com/unkcpz/aiida-optimade/tree/update/aiida-v2#optimade-api-implementation-for-aiida for how it looks like

CasperWA

The matrix looks nice - although it's a bit confusing with the naming, so I changed that a bit. Also, there is no record of this repository pre v0.18, which is a bit confusing as well, but I suppose it's not that relevant.

README.md

unkcpz · 2022-08-25T22:55:50Z

@CasperWA thanks! All changed.

README.md

CasperWA

Great work @unkcpz !

CasperWA · 2022-09-05T12:38:04Z

I'll add a fix for the CI to use optimade version 0.18 instead of the new 0.19.

This is temporary until aiida-optimade works with OPTIMADE Python tools v0.19 and above.

unkcpz · 2022-09-05T12:59:34Z

@CasperWA thanks! But seems the change fail the tests, any idea? I can have a close look into it later today, or do you already have a clue and working on it?

CasperWA · 2022-09-05T13:02:39Z

@CasperWA thanks! But seems the change fail the tests, any idea? I can have a close look into it later today, or do you already have a clue and working on it?

Yeah - I have no idea why, response_fields should be a valid query parameter... And at least it should work with the older version.

CasperWA · 2022-09-05T13:03:48Z

It seems OPTIMADE Python tools has the same issue

Indeed, a work-around is underway, see Materials-Consortia/optimade-python-tools#1317.

ml-evs · 2022-09-05T14:44:30Z

It seems OPTIMADE Python tools has the same issue

Indeed, a work-around is underway, see Materials-Consortia/optimade-python-tools#1317.

New pydantic release is due in a few minutes, so the old Dockerfile will work again. I also pinned the Docker deps in Materials-Consortia/optimade-python-tools#1315 so this problem shouldn't occur again.

unkcpz · 2022-09-05T14:46:20Z

@ml-evs thanks a lot! I retrigger the tests, hope it works fine now.

ml-evs · 2022-09-05T16:12:48Z

I just released v0.19.2 which includes the fix for self-built docker images (which I think is what you are doing here) and also the 2D structure CIF export bug that was reported at the workshop. I'd recommend upgrading!

unkcpz · 2022-09-05T16:23:06Z

@ml-evs, thanks! I will check the update to 0.19.2 which seems break something as show in #375
However, now as here @CasperWA sticking the validator version to 0.18.0 and it should work, no?

ml-evs · 2022-09-05T16:28:54Z

@ml-evs, thanks! I will check the update to 0.19.2 which seems break something as show in #375 However, now as here @CasperWA sticking the validator version to 0.18.0 and it should work, no?

The 3.7 test failures are caused by us dropping Python 3.7 support, which we did mostly because AiiDA 2.0 also dropped it 😅 The other tests look like you just need to pass through the config value for meta.schemas in some of your custom router methods. I can patch this if you like (see Materials-Consortia/optimade-python-tools#1210); though I guess you may want to do it outside of this PR!

unkcpz · 2022-09-05T16:42:33Z

I don't understand why this is related to py3.7, the CI docker test is all run for py3.8.
Please feel free to add a patch to fix the CI test here, thanks a lot!

ml-evs · 2022-09-05T16:47:10Z

I don't understand why this is related to py3.7, the CI docker test is all run for py3.8. Please feel free to add a patch to fix the CI test here, thanks a lot!

I only meant for the other PR you linked, #375. Here, everything should be fixed if you change validator_version to latest (or 0.19) rather than 0.18.0

unkcpz · 2022-09-05T16:49:36Z

I see, thank you! I'll take care of that.

This reverts commit dd1b909.

CasperWA · 2022-09-06T07:01:39Z

I see, thank you! I'll take care of that.

I just reverted my latest "fix" then - I do indeed think we need to add the schema parameter in some places - however, that should really coincide with the update to OPTIMADE Python tools v0.19 (i.e., #375). Also, we should definitely use the new Docker image created in the OPTIMATE Python tools repository (see here).
Although the latter may not be possible due to the extra Docker configurations.

unkcpz · 2022-09-06T07:32:31Z

Also, we should definitely use the new Docker image created in the OPTIMATE Python tools repository (see here).

I agree. I open an issue at #379. Not sure if all the image parameters are configurable. I have used it to replace the dedicated branch in materials cloud for optimade-index

unkcpz marked this pull request as draft March 9, 2022 02:04

unkcpz force-pushed the update/aiida-v2 branch from 038f31f to 9e3ce45 Compare March 9, 2022 02:11

CasperWA requested changes Mar 9, 2022

View reviewed changes

.github/workflows/ci_tests.yml Outdated Show resolved Hide resolved

unkcpz force-pushed the update/aiida-v2 branch from 9e3ce45 to 3aa7332 Compare March 22, 2022 17:23

unkcpz marked this pull request as ready for review April 6, 2022 08:18

unkcpz force-pushed the update/aiida-v2 branch 2 times, most recently from 48f6725 to ea40e21 Compare April 20, 2022 14:18

unkcpz requested a review from CasperWA April 20, 2022 15:47

unkcpz force-pushed the update/aiida-v2 branch 2 times, most recently from 75adadb to 026e96b Compare May 13, 2022 10:18

CasperWA requested changes May 25, 2022

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

aiida_optimade/cli/__init__.py Show resolved Hide resolved

aiida_optimade/cli/cmd_init.py Show resolved Hide resolved

aiida_optimade/translators/entities.py Outdated Show resolved Hide resolved

requirements.txt Outdated Show resolved Hide resolved

unkcpz commented May 25, 2022

View reviewed changes

unkcpz requested a review from CasperWA May 25, 2022 22:05

unkcpz force-pushed the update/aiida-v2 branch 2 times, most recently from 1883537 to c536639 Compare May 30, 2022 09:59

unkcpz mentioned this pull request May 31, 2022

New StructureData added and getting an 500 Internal server error #358

Open

unkcpz force-pushed the update/aiida-v2 branch from c08ca10 to 12600d3 Compare May 31, 2022 16:54

CasperWA requested changes Aug 19, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

review dep matx

4ea0e9a

unkcpz force-pushed the update/aiida-v2 branch from 4226d8e to 4ea0e9a Compare August 25, 2022 22:53

unkcpz requested a review from CasperWA August 25, 2022 22:55

CasperWA requested changes Sep 5, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

r

9181c1d

unkcpz requested a review from CasperWA September 5, 2022 11:54

CasperWA previously approved these changes Sep 5, 2022

View reviewed changes

Use OPTIMADE validator with v0.18.0

dd1b909

This is temporary until aiida-optimade works with OPTIMADE Python tools v0.19 and above.

CasperWA dismissed their stale review via dd1b909 September 5, 2022 12:43

unkcpz requested a review from CasperWA September 5, 2022 22:16

Revert "Use OPTIMADE validator with v0.18.0"

e7255ed

This reverts commit dd1b909.

CasperWA approved these changes Sep 6, 2022

View reviewed changes

unkcpz merged commit b3caba2 into aiidateam:develop Sep 6, 2022

Migrate to aiida-core 2.0 #331

Migrate to aiida-core 2.0 #331

Conversation

unkcpz commented Mar 9, 2022 • edited Loading

codecov bot commented Mar 9, 2022 • edited Loading

Codecov Report

CasperWA left a comment

Choose a reason for hiding this comment

CasperWA commented Mar 9, 2022

unkcpz commented Apr 20, 2022

unkcpz commented Apr 20, 2022 • edited Loading

eimrek commented May 25, 2022

unkcpz commented May 25, 2022

CasperWA commented May 25, 2022

CasperWA left a comment

Choose a reason for hiding this comment

CasperWA commented May 25, 2022 • edited Loading

eimrek commented May 25, 2022

CasperWA commented May 25, 2022

unkcpz left a comment

Choose a reason for hiding this comment

unkcpz May 25, 2022

Choose a reason for hiding this comment

sphuber May 26, 2022

Choose a reason for hiding this comment

CasperWA Jun 7, 2022

Choose a reason for hiding this comment

unkcpz commented May 31, 2022 • edited Loading

unkcpz commented May 31, 2022 • edited Loading

unkcpz commented May 31, 2022

unkcpz commented Aug 17, 2022

CasperWA left a comment

Choose a reason for hiding this comment

unkcpz commented Aug 25, 2022

CasperWA left a comment

Choose a reason for hiding this comment

CasperWA commented Sep 5, 2022

unkcpz commented Sep 5, 2022

CasperWA commented Sep 5, 2022

CasperWA commented Sep 5, 2022 • edited Loading

ml-evs commented Sep 5, 2022

unkcpz commented Sep 5, 2022

ml-evs commented Sep 5, 2022

unkcpz commented Sep 5, 2022

ml-evs commented Sep 5, 2022

unkcpz commented Sep 5, 2022

ml-evs commented Sep 5, 2022

unkcpz commented Sep 5, 2022

CasperWA commented Sep 6, 2022 • edited Loading

unkcpz commented Sep 6, 2022

unkcpz commented Mar 9, 2022 •

edited

Loading

codecov bot commented Mar 9, 2022 •

edited

Loading

unkcpz commented Apr 20, 2022 •

edited

Loading

CasperWA commented May 25, 2022 •

edited

Loading

unkcpz commented May 31, 2022 •

edited

Loading

unkcpz commented May 31, 2022 •

edited

Loading

CasperWA commented Sep 5, 2022 •

edited

Loading

CasperWA commented Sep 6, 2022 •

edited

Loading