search

ezpzbz / aiida-orca

AiiDA Plugin for ORCA
MIT License
7 stars 4 forks source link

Support aiida-2.0 #46

Closed danielhollas closed 1 year ago

danielhollas commented 1 year ago

Some minor and backwards-compatible changes to support AiiDA 2.0. With this PR we can easily support both AiiDA 1.0 and 2.0 at the same time.

Due to unrelated issues in AiiDAlab environment, I had problems with Pymatgen. I looked around and we could get rid of the Pymatgen dependency (and in turn many other transitive dependencies from aiida[atomic_tools]) and use ASE instead for parsing. ASE seems to have much less dependencies,

$ pip show ase
Name: ase
Version: 3.19.3
Requires: matplotlib, numpy, scipy
$ pip show pymatgen
Name: pymatgen
Version: 2022.2.1
Summary: Python Materials Genomics is a robust materials analysis code that defines core object representations for structures and molecules with support for many electronic structure codes. It is currently the core analysis code powering the Materials Project (https://www.materialsproject.org).
Requires: Cython, matplotlib, monty, networkx, numpy, palettable, pandas, plotly, pybtex, requests, ruamel.yaml, scipy, spglib, sympy, tabulate, tqdm, uncertainties

I tried to test orca_asa as well, but unfortunately the orca_asa module is broken in ORCA >5.0 (segfaults) so I couldn't test it. The bug was reported a year ago and still not fixed detail at https://orcaforum.kofo.mpg.de/viewtopic.php?f=11&t=7716&p=40653#p40653. The recommendation is to switch to the new orca_esd module.

TODO:

Closes #35

danielhollas commented 1 year ago

Turning on AiiDA2 deprecation warnings with export AIIDA_WARN_v3=1 results only on these warnings:

  /home/jovyan/aiida-orca/aiida_orca/parsers/__init__.py:38: AiidaDeprecationWarning: `FolderData.open` is deprecated, use `FolderData.base.repository.open` instead. (this will be removed in v3)
    with self.retrieved.open(fname_out) as handler:

  /home/jovyan/aiida-orca/aiida_orca/parsers/__init__.py:73: AiidaDeprecationWarning: `FolderData.open` is deprecated, use `FolderData.base.repository.open` instead. (this will be removed in v3)
    with out_folder.open(fname_relaxed) as handler:

I am ignoring these in this PR so that we can support both AiiDA v1.x and v2.x.

sphuber commented 1 year ago

You should delete the .github/workflows/build.yml file in this branch. It was deleted in the previous PR. Same goes for pytest.ini.

danielhollas commented 1 year ago

@sphuber ah, thanks. I messed up the merge I guess.

ezpzbz commented 1 year ago

Hi @danielhollas and @sphuber Thanks very much for the PRs. Sorry been hectic couple of months. I do not have much experience with AiiDA v2.0 to comment on the code. The only note on my side is that let's have a release for the AiiDA v1.0 compatible version and have it in a separate branch rather than develop before merging this PR into develop.

danielhollas commented 1 year ago

Hi @pzarabadip. I don't think a separate branch is needed, since we can easily support 1.0 and 2.0 versions, as is done in this PR and already on the develop branch. I would therefore prefer to merge these PRs first, and then release a new version if that is okay with you.

(Not that we have some functioning CI tests, it is now easier to make sure we support both versions, just need to merge in latest changes into this branch)

ezpzbz commented 1 year ago

Hi @pzarabadip. I don't think a separate branch is needed, since we can easily support 1.0 and 2.0 versions, as is done in this PR and already on the develop branch. I would therefore prefer to merge these PRs first, and then release a new version if that is okay with you.

(Not that we have some functioning CI tests, it is now easier to make sure we support both versions, just need to merge in latest changes into this branch)

Hi @danielhollas I'd like to have a latest aiida-1 compatibale version pushed in a branch such as aiida-1. That makes using and developing it much less painful (if needed). There is no harm in having it. I see no reason why not having a release for latest aiida-1 compatible version. Then, this aiida-2 one will sit in develop and with stable one in main.

sphuber commented 1 year ago

The only suggestion from my side is splitting the CI into aiida-1 and aiida-2. Besides that rest all good to me. Thanks very much.

Just for my understanding, do you mean to have two separate yml files with each an individual workflow? What is the advantage in your mind of having to separate CI workflows?

ezpzbz commented 1 year ago

The only suggestion from my side is splitting the CI into aiida-1 and aiida-2. Besides that rest all good to me. Thanks very much.

Just for my understanding, do you mean to have two separate yml files with each an individual workflow? What is the advantage in your mind of having to separate CI workflows?

Hi @sphuber. I thought of this as if we separate a separate branch for aiida-1-only version of the plugin. Hence, if the development version does not require checking against aiida-1, we can drop them in CI here.

sphuber commented 1 year ago

Hi @sphuber. I thought of this as if we separate a separate branch for aiida-1-only version of the plugin. Hence, if the development version does not require checking against aiida-1, we can drop them in CI here.

I see, but I though @danielhollas idea was to keep a single branch/version that is compatible with 1.6.x and v2.x at the same time, which is currently the idea. The changes between 1.x and 2.x were kept minimal on purpose and almost all changes were backwards-compatible, with just deprecation warnings being emitted. So just to say that at least technically, you don't need two separate branches to support both versions

danielhollas commented 1 year ago

First of all, my apologies that this PR is a bit of a mess. While it originally started as a PR to support AiiDA-2 (in a backwards compatible manner), it accrued a bunch of other unrelated changes. I've decided to split it up into separate PRs to aid review and make the Git history cleaner. CI related changes are in #55, the switch from Pymatgen to ASE in #54, and the rest will follow.

Additionally, in the meantime due to the kind help of @sphuber, the changes that were actually needed to support 2.0 were already merged to the develop branch as part of his PRs that introduced basic tests in CI. The only thing left in this PR for 2.0 support is relaxing the aiida-core requirement in pyproject.toml (and some fixes in examples/*py). All other changes here are unrelated and apply equally to AiiDA 1.x and 2.x.

Hence, as @sphuber mentioned, I really don't see a need why we should split the development now. It would only make things more complicated, since every change would need to be backported to the 1.x branch as well. Note that all the currently opened PRs from me are compatible with 1.x, and I don't see any immediate need to make changes that would be incompatible with 1.x in the near future.

sphuber commented 1 year ago

@danielhollas you want me to review #54 and #55 ?

danielhollas commented 1 year ago

@sphuber oh, thanks for the offer. Would of course love to have your eyes on the code, but feel free to skip if you're busy.

ezpzbz commented 1 year ago

Thanks @danielhollas and @sphuber. I was thinking that this version is incompatible with aiida-1. As it is not the case, I'm happy going with it as it is. @danielhollas thanks for separating the commits into different PRs.

danielhollas commented 1 year ago

I've opened the last two PRs #56 and #57 that are based on this one. This one can therefore be closed.