Adapt modules and files layout for plugin implementation

[unkcpz]

This PR is separated and prepared for #428 to avoid mixing changes, especially for moving modules/files without changing any implementations. See #428 (comment) for the design of restructuring the modules.

Every commit serves its own purpose, so you can basically review it commit by commit.

From #428, there are still more things that should be separated:

• some unit tests which not related to the new "plugin" design can be implemented first to test widgets.
• ipynb can be simplified and moved to a module for test purposes as did in Plugin UI #428

But better to use new PRs as well. Do you want to do it yourself @superstar54 (maybe better since it was all your effort and it might not fair to "plagiarise" it by me) or I can do it and put you as co-author.

[codecov]

Codecov Report

Patch coverage: 62.18% and project coverage change: +0.81% 🎉

Comparison is base (823a6fb) 52.61% compared to head (e0c545e) 53.42%.

❗ Current head e0c545e differs from pull request most recent head d2b107a. Consider uploading reports for the commit d2b107a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #439      +/-   ##
==========================================
+ Coverage   52.61%   53.42%   +0.81%     
==========================================
  Files          17       26       +9     
  Lines        2068     2104      +36     
==========================================
+ Hits         1088     1124      +36     
  Misses        980      980              

Flag	Coverage Δ
python-3.10	53.42% <62.18%> (+0.81%)	⬆️
python-3.8	53.47% <62.18%> (+0.81%)	⬆️
python-3.9	53.47% <62.18%> (+0.81%)	⬆️

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

Files Changed	Coverage Δ
src/aiidalab_qe/__init__.py	100.00% <ø> (ø)
src/aiidalab_qe/app/common/process.py	30.23% <ø> (ø)
src/aiidalab_qe/app/common/widgets.py	31.74% <ø> (ø)
src/aiidalab_qe/app/configuration/pseudos.py	92.85% <ø> (ø)
src/aiidalab_qe/app/parameters/__init__.py	100.00% <ø> (ø)
src/aiidalab_qe/app/result/report.py	28.12% <ø> (ø)
src/aiidalab_qe/app/structure/__init__.py	36.48% <ø> (ø)
src/aiidalab_qe/version.py	100.00% <ø> (ø)
src/aiidalab_qe/workflows/__init__.py	48.14% <ø> (ø)
src/aiidalab_qe/app/result/electronic_structure.py	14.06% <14.06%> (ø)
... and 16 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[superstar54]

@unkcpz Thanks for the work. LGTM!

One thing we may consider is to remove the app folder and change it to:

~/apps/aiidalab-qe/src/aiidalab_qe$ ls
common         __init__.py  parameters  static     submission  workflows
configuration  __main__.py  result      structure  version.py

so that the configuration, submission, and common are at the same level as the workflow. @AndresOrtegaGuerrero what do you think?

For the test and ipynb, I think I have time to do it next week.

[unkcpz]

No really, see the discussion we had at #379 (comment). Put workflow folder separately will make the workflow import faster without importing anything from app module.

[unkcpz]

@superstar54 there are still two things that can be done separately before start introduce the "Panel" concept in:

• some unit tests which not related to the new "plugin" design can be implemented first to test widgets.
• ipynb can be simplified and moved to a module for test purposes as did in Plugin UI #428

Can I also do it? This will make me to have a close line-by-line investigation on how you implement tests on every widgets and what are not covered.

[superstar54]

No really, see the discussion we had at #379 (comment). Put workflow folder separately will make the workflow import faster without importing anything from app module.

This can be avoided. Just let the aiidalab_qe/__init__.py empty. There is no difference if you put the folder inside the app or not.
In the future PR related to the workchain. we need to make the aiidalab_qe/__init__.py or the aiidalab_qe/app/__init__.py empty to avoid recursive import.

[superstar54]

@superstar54 there are still two things that can be done separately before start introduce the "Panel" concept in:

• some unit tests which not related to the new "plugin" design can be implemented first to test widgets.
• ipynb can be simplified and moved to a module for test purposes as did in Plugin UI #428

Can I also do it? This will make me to have a close line-by-line investigation on how you implement tests on every widgets and what are not covered.

I only have time to do these until next Monday. If you have time do these before it. Please do it.

[unkcpz]

This can be avoided. Just let the aiidalab_qe/init.py empty.

Okay, I see your point. I'll benchmark and change if import workflow still fast.

[unkcpz]

Move the modules out and clean the __init__.py, all good. Here is the load time check:

# workflows module separated from `app`
(base) jovyan@51e7296639d5:~$ time python -c "from aiidalab_qe.workflows import QeAppWorkChain"

real    0m1.897s
user    0m2.443s
sys     0m3.717s

# workflow and app in the same hierarchy, and entries in `__init__.py`
(base) jovyan@51e7296639d5:~$ time python -c "from aiidalab_qe.workflows import QeAppWorkChain"

real    0m3.201s
user    0m3.679s
sys     0m3.833s

#  workflow and app in the same hierarchy,, and no entries in `__init__.py`
(base) jovyan@51e7296639d5:~$ time python -c "from aiidalab_qe.workflows import QeAppWorkChain"

real    0m1.682s
user    0m2.176s
sys     0m3.515s

[unkcpz]

However, I still don't think this is good. This means we cannot/should not put any module to aiidalab_qe/__init__.py, put any module from the current app will slow down the import of workflows module, which is what we want. @superstar54 if this is not what your plugin implementation needed, I propose we just keep it as it is now?

[superstar54]

This means we cannot/should not put any module to aiidalab_qe/init.py,

In the current app and workflow structure, you also should not put anything in the aiidalab_qe/__init__.py, right? You put all the app-related modules into the aiidalab_qe/app/__init__.py, and then import them like this:

from aiidalab_qe.app import QeApp

you can not import a module from aiidalab_qe directly, right? In this case, the app folder is really not necessary.
e.g, one need to always add the app when importing
from aiidalab_qe.app.configuration import Configuration

I think it's better to do it like this:
from aidalab_qe.configuration import Configuration

@superstar54 if this is not what your plugin implementation needed, I propose we just keep it as it is now?

In the workchain PR, to avoid recursive import, one should not put QeApp class in the app/__init__.py, which means I need to create a file app/app.py, which is not good. So I suggest to remove the app folder and creating an app.py file directly.

[unkcpz]

you can not import a module from aiidalab_qe directly, right?

You miss the goal here, we separate the workflows folder to avoid importing QeAppWorkChain is slow. As the command, I used above for benchmarking: time python -c "from aiidalab_qe.workflows import QeAppWorkChain"

e.g, one need to always add the app when importing
from aiidalab_qe.app.configuration import Configuration
I think it's better to do it like this:
from aidalab_qe.configuration import Configuration

Neither of them are good, ultimately it is more clear to have from aiidalab_qe.app import Configuration, so I want to put things in aiidalab_qe/app/__init__.py.

In the workchain PR, to avoid recursive import, one should not put QeApp class in the app/init.py, which means I need to create a file app/app.py

The QeApp class is the class that is not dependent on other class/modules, if there are recursive import, you need to check your implementation to avoid that, no?
I suggest keeping this structure and I start also moving to QeApp, maybe I'll see the problem then.

[AndresOrtegaGuerrero]

@unkcpz thank you ! , It looks good to me. I am wondering what have you decided about the apps folder. I am pro reducing the time of loading of the app, since it could be a lil slow. I am not too knowledgable in this conversation. I was reading about parallel loading using concurrent.futures, Do you think that could be worth it to try (please if i am saying some dumb just let me know :D )

[unkcpz]

I was reading about parallel loading using concurrent.futures, Do you think that could be worth it to try

I don't think this is possible, it runs things asynchronously which still uses a single thread, the module load is not ID intense tasks and thus will no benefit from it. Moreover, it is not designed to import the modules.

[unkcpz]

The rebase of #435 on this are quite light, only 3 files need to solve the conflict. Which means the refactoring is well recognized by git.