-
Notifications
You must be signed in to change notification settings - Fork 15
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
Trigger the update of node view #844
Trigger the update of node view #844
Conversation
0f02f01
to
7f884e4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #844 +/- ##
==========================================
+ Coverage 68.32% 68.35% +0.03%
==========================================
Files 50 50
Lines 4568 4573 +5
==========================================
+ Hits 3121 3126 +5
Misses 1447 1447
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
f8c7338
to
f6f1c89
Compare
f6f1c89
to
0ac137f
Compare
Here is an example to show the result tabs are automatically generated when the process is finished. Please go to ~ 30 seconds of the vedio. qeapp-update-result-tabs.mp4 |
self._process_monitor = ProcessMonitor( | ||
process=self.node, | ||
self.process_monitor = ProcessMonitor( | ||
timeout=1.0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How this timeout works ?, Could it lead to any memory issue for a workflow that takes long ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the moment, the ProcessMonitor
will call the callback functions every timeout seconds. This, indeed, will be a problem if the callback function is computationally expensive.
In principle, we only need to run the callback function when the process is finished (or even better, when the process is modified). I have opened an issue in the aiidalab-widget-base.
We can wait for that issue to be solved before merging this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or at least we should increase the time , 1 second is to often
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use a small value for the timeout, so that the results can be immediately shown when the process is finished.
As pointed out by @danielhollas , there is a on_sealed
argument for the callbacks. I changed the callback to on_sealed
, so that the _update_view
method will only call when the process is finished. In this case, the timeout 1 s should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! thank you Xing
This PR fixes the process monitor to update the result tabs.
The ideal solution is: as long as the process is updated (e.g., has a new output), we trigger the update of the node view. However this is not possible in the current
ProcessMonitor
. Therefore, we only trigger the update when the process is finished.I opened an issue on the
ProcessMonitor
.