seprate workchain selector to be extendable #401
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #401 +/- ##
==========================================
- Coverage 50.66% 50.53% -0.14%
==========================================
Files 16 16
Lines 1804 1789 -15
==========================================
- Hits 914 904 -10
+ Misses 890 885 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@unkcpz this looks great, thanks! Let's try it out here, and then later we can upstream to AWB.
Except for some minor comments / suggestions that you can feel free to ignore, there is one subtle thing about triggering a refresh upon submitted WorkChain completion.
qe.ipynb
Outdated
There was a problem hiding this comment.
If you look closely at the corresponding PR in my app, I've added one extra hack, where I trigger the workchain selector refresh when a submitted workflow finishes. That way the process status in the selector is updated from "Running" to "Finished". This is a bit ugly since I need to directly patch the ProcessMonitor object in one of the Step classes, the but it works.
There was a problem hiding this comment.
Interesting, this is cool. I also mad I change it back because after look at the process_monitor the interface of ViewQeAppWorkCHainStatusAndStep so it is designed to be used as such, then it is not very hacked.ProcessMonitor this on_seal is not very clear to me. I think it is more better to first to make the process monitor properly interfaced and have a way no notify another widget with the process is finished.
There was a problem hiding this comment.
Fair enough. The on_sealed callbacks are called when the process finishes. Normally one should passed them via the class constructor, so what I am doing is definitely hacky, since I am touching the internal implementation of ProcessMonitor.
|
@unkcpz since there is already an associated issue, I've removed this PR from the AiiDAlab project to avoid duplication. Hope that's okay. |
|
Sure, thanks! If you mean it is duplicated with #388 which is already in the current sprint. I was adding this to the project in order to set it to the current sprint so I didn't forget to bring it up in the meeting (I admit this is not the original way we plan to use the sprint, but for myself it a bit easy to track everything happened. I start to have a very bad memory among a couple of things 😿 ). |
There was a problem hiding this comment.
@unkcpz have you tested your latest changes? Found a bug while I was transplanting this into my app.
unkcpz
force-pushed
the
fea/separate-workchain-selector
branch
from
ffef39a
to
de5083a
Compare
As mentioned in #388, the workchain selector is one of the sources lead to the memory leak. It uses a spare threading to update the list which is not fully necessary because the `refresh` button is already served for this purpose. In this PR we did not only remove the threading but also decouple the QeAppWorkChain specific code out as the sub-class, so the WorkChainSelector can be re-used through inherited by other apps.
fixes #388
This PR is also an attempt of aiidalab/aiidalab-widgets-base#382.