0x1: Move QeAppWorkChain into the workflow module

[unkcpz]

As we discussed, it is not needed now to have a separated package for the work chain plugin.
This will make our life easier to modify the work chain. I test on with running some simple calculations locally over QeApp, seems all fine.

• update bumpver
• remove workchain package installation from requirements

[mbercx]

Thanks @unkcpz! Changes look fine, but the Chrome integration tests are failing? Not very familiar with these I'm afraid, so no idea what's going wrong there.

[danielhollas]

I would have one suggestion, and that is to still have the workchain as a separate package, but install it together with aiidalab_qe. When i was trying to do the same change as here in my app, I ran into issues with AiiDA daemon, because it ends up importing the whole aiidalab_qe package instead of just the workchain.

There are different ways to do it, one way would be to have two packages in the src directory.

src/aiidalab_qe/
src/aiidalab_qe_workchain/

Another, somewhat more complicated way would be to use namespace packages.

A good page about various possible layouts is in the setuptools docs

https://setuptools.pypa.io/en/latest/userguide/package_discovery.html

[unkcpz]

When i was trying to do the same change as here in my app, I ran into issues with AiiDA daemon, because it ends up importing the whole aiidalab_qe package instead of just the workchain.

@danielhollas thanks for the feedback. I am not sure what is the issue exactly. In my test, I don't have a problem with import. I think you can even more safe to make the workchain an aiida plugin endpoint and use the class by sort of like QeAppWorkChain = WorkflowFactorory('aiidalab_qe.workflows.qeapp').

[unkcpz]

Thanks @unkcpz! Changes look fine, but the Chrome integration tests are failing. Not very familiar with these I'm afraid, so no idea what's going wrong there.

@mbercx, that happened sometimes because the waiting time is too short that the selenium request is not fully loaded (This is what needs to be improved, but it is independent of this change here. I open an issue in #380). I retrigger the test and it passes.

[AndresOrtegaGuerrero]

Thanks @unkcpz , I tested the branch in a new container. The only issue I experienced is that once i did pip install -e . on the branch, I needed an extra verdi daemon restart to avoid ImportError: module 'aiidalab_qe.workflows' from identifier 'aiidalab_qe.workflows:QeAppWorkChain' could not be loaded. Once i did data the workchain worked with no issues. Do you think this need to be addressed ?

[unkcpz]

@AndresOrtegaGuerrero thanks for the test.

I needed an extra verdi daemon restart to avoid ImportError: module 'aiidalab_qe.workflows' from identifier 'aiidalab_qe.workflows:QeAppWorkChain' could not be loaded

I don't think this is a problem. The original approach which split the packages, you also need to restart the daemon. And I think you only need to do this the first time after you install the app. From user's perspective, the aiidalab install will manage the daemon restart so this won't be an issue in production.

[AndresOrtegaGuerrero]

@AndresOrtegaGuerrero thanks for the test.

I needed an extra verdi daemon restart to avoid ImportError: module 'aiidalab_qe.workflows' from identifier 'aiidalab_qe.workflows:QeAppWorkChain' could not be loaded

I don't think this is a problem. The original approach which split the packages, you also need to restart the daemon. And I think you only need to do this the first time after you install the app. From user's perspective, the aiidalab install will manage the daemon restart so this won't be an issue in production.

From my side then it can be merged

[AndresOrtegaGuerrero]

Looks fine to me

[unkcpz]

Hi @danielhollas, do you want to have a second review on this? Maybe what happened to your app is another issue?

[danielhollas]

I will soon be doing a similar refactoring in my app, and will report what I found. I think regardless it make sense to keep the workchain in a separate package (but within the same setup.cfg, installable together) so that aiidalab and aiida workchain always remain uncoupled, and so that aiida daemon does not import ipython/aiidalab/awb stuff. But if it works for you this otherwise looks good and you can merge if you don't want to wait for my investigation.

[unkcpz]

Thanks! No in hurry. I'll wait until you do the refactoring and see which way of arranging modules is more clear.

[superstar54]

When i was trying to do the same change as here in my app, I ran into issues with AiiDA daemon, because it ends up importing the whole aiidalab_qe package instead of just the workchain.

I don't think this is an issue. When one imports the workchain by:

from aiidalab_qe.workflows import QeAppWorkChain

It will import all modules inside the aiidalab_qe.__init__.py file. Since AiiDA and AiiDAlab-qe are installed in the same environment, it is not an issue for AiiDA daemon to import these modules. One downside is that this may take time, but will not exceed a few seconds.

[danielhollas]

It will import all modules inside the aiidalab_qe.init.py file.

Indeed, that is an issue. For my app, it looks like import itself takes around 3 seconds, while just importing the workchain itself is only 1 second. Also, importing the app ends up importing a lot of stuff, the whole AWB and its dependencies, aiidalab package etc. If one is not careful, there might be side-effects. In my case, I was using a Bokeh command output_notebook during import, which somehow activated the ipython environment and had weird effects.

In any case, I think it makes sense to try to decouple the AiiDA workchains from AiiDAlab package, so I ended app having it in a separate subpackage in a way that I can import it separately.

aiidalab_ispg/
    __init__.py
    app/
    app/__init__.py
    workflows/
    workflows/__init__py

Note that the top-level __init__.py is mostly empty, so that import aiidalab_ispg.workflows does not trigger the import of aiidalab_ispg.app.

I am not saying this is the best solution, but that's what I ended up doing for now.

https://github.com/ispg-group/aiidalab-ispg/tree/main/aiidalab_ispg

I also considered using namespace packages, but it seems a bit too complicated for my use case.

[unkcpz]

I think it makes sense to try to decouple the AiiDA workchains from AiiDAlab package, so I ended app having it in a separate subpackage in a way that I can import it separately.

As a subpackage seems a compromised (and better) solution, will do that.

[unkcpz]

I re-organize the modules as @danielhollas suggested. Test locally running a band structure calculation, and works as expected.
@danielhollas can you have a second look and I think it is ready to be merged.

[unkcpz]

I merge this for now, it kind of blocks me for finalize the refactoring. We are pretty much safe since we are not aggressively make new releases.

[danielhollas]

@unkcpz apologies for late review, I was on holidays last week.

[danielhollas]

You probably want to move this file to app/ Otherwise, when you import aiidalab_qe.workflows you'll end up importing aiidalab_qe.app as well. The top level init file should probably only export __version__.

[unkcpz]

@unkcpz apologies for late review, I was on holidays last week.

No worries, I have to merge it because it was a pain to rebase and continue with #382.

I see, thanks @danielhollas. Will do this in another PR.