Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code/Pseudos setup: Separate frontend from backend #796

Merged
merged 5 commits into from
Aug 19, 2024

Conversation

danielhollas
Copy link
Contributor

@danielhollas danielhollas commented Aug 13, 2024

This is a preparatory PR for refactoring the code setup. It is imho better to separate the frontend (widgets that are displayed in the app) from the routines that actually install stuff in the backend and in the CLI. In this PR, I only move code, and don't do any actual changes yet.

As an immediate advantage, just executing python -m aiidalab_qe --help got noticeably faster since we no longer import the rest of the app.

In the next PR, instead of importing the install routines in the widgets, we can simply invoke the CLI from a subprocess. That way we do not need to worry about the threading issues, which in turn will allow us to get rid of generating the code setup code and execute it directly.

@danielhollas danielhollas marked this pull request as ready for review August 13, 2024 22:46
@danielhollas danielhollas mentioned this pull request Aug 13, 2024
4 tasks
Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 58.60215% with 77 lines in your changes missing coverage. Please review.

Project coverage is 68.19%. Comparing base (ea6b645) to head (ffc85f4).
Report is 2 commits behind head on main.

Files Patch % Lines
src/aiidalab_qe/setup/codes.py 22.89% 64 Missing ⚠️
src/aiidalab_qe/setup/pseudos.py 90.80% 8 Missing ⚠️
src/aiidalab_qe/__main__.py 37.50% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #796      +/-   ##
==========================================
- Coverage   68.22%   68.19%   -0.03%     
==========================================
  Files          45       48       +3     
  Lines        4148     4157       +9     
==========================================
+ Hits         2830     2835       +5     
- Misses       1318     1322       +4     
Flag Coverage Δ
python-3.10 68.19% <58.60%> (-0.03%) ⬇️
python-3.9 68.23% <58.60%> (-0.03%) ⬇️

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

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

@unkcpz
Copy link
Member

unkcpz commented Aug 19, 2024

Thanks @danielhollas, would you mind to separate the renaming in to a commit, it is a bit hard to see what had changed.

@danielhollas
Copy link
Contributor Author

Thanks @danielhollas, would you mind to separate the renaming in to a commit

I am not quite sure what you mean, and not sure how to separate it. It's not really renaming, it's a code split. The backend code was moved from aiidalab_qe.common to aiidalab.setup.

For example, most of the code from aiidalab_qe/src/common/setup_code.py has been moved to aiidalab_qe/src/setup/code.py, except for the QeSetupWidget. Does that make sense?

@unkcpz
Copy link
Member

unkcpz commented Aug 19, 2024

Ah, okay, I thought it was rename and changes. If it is split, then nothing too much can be simplified. I am reviewing it.

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @danielhollas, looking forward to see next PR how you invoke from CLI to avoid threading issue.

@danielhollas danielhollas merged commit 8855969 into aiidalab:main Aug 19, 2024
8 checks passed
@danielhollas danielhollas deleted the refactor-cli branch August 19, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants