UUID as traitlets for threading related widgets #375

unkcpz · 2022-10-21T18:05:55Z

The widget changed are:

ComputationalResourcesWidget because of QeApp code setup trigger refresh it in thread.
ComputerDropdownWidget
ProcessListWidget because thread call in update function for process.
ProcessMonitor thread call in join
ProcessNodesTreeWidget

danielhollas · 2022-10-27T21:00:58Z

@unkcpz sorry for a delay on this. I'll try to take a closer look and test this next week. Thanks!

unkcpz · 2022-11-01T11:16:26Z

@unkcpz sorry for a delay on this. I'll try to take a closer look and test this next week. Thanks!

Thanks! Since this PR is separated from the migration to 2.x and it is not manifest the thread issue in 1.6.x, I think you can have a test to make sure everything is working with the new changes in this PR.

danielhollas · 2022-11-02T16:37:38Z

I made some progress today, was able to convert my app to support this branch and did some basic testing. Will try to do more testing tomorrow.

unkcpz · 2022-11-07T13:58:05Z

Hi @danielhollas, do you have any update on using and testing this? I want to try to finalize this and hopefully have QeApp support AiiDA 2.x supported by next week.

aiidalab_widgets_base/computational_resources.py

danielhollas

@unkcpz here's my initial review. I agree with the target of merging this next week.

I did some testing within my own application, but need to do a bit more and want to test individual widgets as well.

I have also ran into an interesting edge case that possibly not covered here. I had a widget that passed an AiiDA node to another widget based on the UI interaction (e.g. user clicks a button and AiiDA node is passed to a widget). Presumably because the UI interaction was handled by a different thread than the thread that originally passed the Node, it ran into the same issue as we're solving here, even though the widget itself did not explicitly use threading. So something to be mindful of.

aiidalab_widgets_base/computational_resources.py

danielhollas · 2022-11-07T14:28:12Z

aiidalab_widgets_base/process.py

@@ -545,10 +542,10 @@ class ProcessListWidget(ipw.VBox):

    past_days (int): Sumulations that were submitted in the last `past_days`.

-    incoming_node (int, str, Node): Trait that takes node id or uuid and returns the node that must
+    incoming_node (tr): Trait that takes node uuid that must


Any reason why not keep support of passing id instead of uuid?

You mean pk? Do you have any use case for where you what to use primary key, since I think you can use UUID to include all the cases for pk?

danielhollas · 2022-11-07T14:39:53Z

aiidalab_widgets_base/process.py

-        process = change["new"]
-        if process is None or process.id != getattr(change["old"], "id", None):
+        process_uuid = change["new"]
+        if process_uuid is None or process_uuid != change["old"]:


Can we clean this up a bit to reduce the indentation levels? The first if statement is super confusing to me.

If I understood the logic correctly, this should work?

Suggested change

if process_uuid is None or process_uuid != change["old"]:

if process_uuid == change["old"]:

return

# stop thread (if running)

if self._monitor_thread is not None:

with self._monitor_thread_lock:

self._monitor_thread_stop.set()

self._monitor_thread.join()

if process_uuid is None:

return

with self.hold_trait_notifications():

with self._monitor_thread_lock:

self._monitor_thread_stop.clear()

self._monitor_thread = Thread(

target=self._monitor_process, args=(process_uuid,)

)

self._monitor_thread.start()

Yes, fully agree. I try to simplify it more. Please have a test of it. I'll do the test further more on qeApp.

for more information, see https://pre-commit.ci

unkcpz

@danielhollas thanks a lot for the test. I think the main open issue is whether we need to using only the UUID or also support the other entities (label/pk etc.) as the value of those widgets.

I update the ProcessMornitor to try to make it more clear.

aiidalab_widgets_base/computational_resources.py

aiidalab_widgets_base/process.py

unkcpz · 2022-11-07T16:34:41Z

aiidalab_widgets_base/process.py

@@ -545,10 +542,10 @@ class ProcessListWidget(ipw.VBox):

    past_days (int): Sumulations that were submitted in the last `past_days`.

-    incoming_node (int, str, Node): Trait that takes node id or uuid and returns the node that must
+    incoming_node (tr): Trait that takes node uuid that must


You mean pk? Do you have any use case for where you what to use primary key, since I think you can use UUID to include all the cases for pk?

unkcpz · 2022-11-07T16:52:29Z

aiidalab_widgets_base/process.py

-        process = change["new"]
-        if process is None or process.id != getattr(change["old"], "id", None):
+        process_uuid = change["new"]
+        if process_uuid is None or process_uuid != change["old"]:


Yes, fully agree. I try to simplify it more. Please have a test of it. I'll do the test further more on qeApp.

aiidalab_widgets_base/process.py

unkcpz · 2022-11-10T16:19:00Z

I got this PR tested on QeApp and no issue was found.

danielhollas · 2022-11-10T16:46:00Z

@unkcpz great that you tested it. I didn't get to it this week due to other things. I'll get back to it on Monday, but if you need to merge before than feel free.

Next week I will for sure need to do some production calculations so will test this more, perhaps this can be merged but still not released as a full 2.0 version before more testing is done?

unkcpz · 2022-11-10T16:51:04Z

Next week I will for sure need to do some production calculations so will test this more, perhaps this can be merged but still not released as a full 2.0 version before more testing is done?

Yes, I think we can set some milestones in the next meeting for 2.0.0. I think the major version is for backward compatibility breakup. This means if we change other traitslet, it breaks compatibility. I think we need to make the release after that?

unkcpz · 2022-11-16T11:15:23Z

Hi @yakutovicha, are you reviewing this? I think we can have another alpha release with this merged. Then I can keep on working on QeApp.
@danielhollas, also if you want to have a final check/test.

yakutovicha

Overall looks good to me.

Looking at the discussion, I see one unclear point, whether to allow to use of node labels together with UUIDs. IMO, it would be ok to keep UUIDs only, as I wrote in the review comments.

@danielhollas please let me know if you agree with that.

aiidalab_widgets_base/computational_resources.py

yakutovicha · 2022-11-16T15:11:31Z

aiidalab_widgets_base/process.py

@@ -688,7 +695,7 @@ def start_autoupdate(self, update_interval=10):
 class ProcessMonitor(traitlets.HasTraits):
    """Monitor a process and execute callback functions at specified intervals."""

-    process = traitlets.Instance(ProcessNode, allow_none=True)
+    process_uuid = Unicode(allow_none=True)


What about naming it value?

Changed. Also test the new API in QE app.

danielhollas · 2022-11-16T15:35:32Z

Thanks @unkcpz. I was testing it today / yesterday in my app and did not find any issues, but was not testing all the functionality here. Unfortunately, I had some other urgent stuff to do for tomorrow's meeting. I'll try to do one more round of testing today late evening if I get to it. Otherwise, this is good to merge from my side and release a new alpha version.

@yakutovicha I think you're right about the UUID value, I am find with only supporting UUID. One question, how does the Code selection UI looks if there are two computers with the same label? e.g. if one imports nodes from another DB? Perhaps we should display PKs in the Dropdown in such cases, since otherwise the user will only see two identical labels?

yakutovicha · 2022-11-16T15:40:25Z

One question, how does the Code selection UI looks if there are two computers with the same label? e.g. if one imports nodes from another DB? Perhaps we should display PKs in the Dropdown in such cases, since otherwise the user will only see two identical labels?

I think they are indistinguishable from the current UI. I agree with you that adding a PK number alongside with the code label would be a good option 👍

) - `aiida-core~=2.2` to align with aiida-core version in new docker stack - `aiidalab-widget-base==2.0.0b5` for pinning the widgetnbextension version aiidalab#467 - `widget-bandsplot~=0.5.1` for fixing TDOS background filling issue.

unkcpz added 2 commits October 21, 2022 16:42

computational_resource uuid for traitlets

6c3cae7

uuid for traitlets PrcessFollower and Calc Node tree process

788e2f8

unkcpz requested review from yakutovicha and danielhollas October 21, 2022 18:05

unkcpz mentioned this pull request Oct 21, 2022

Adapt to support uuid traitlets aiidalab/aiidalab-qe#307

Closed

unkcpz mentioned this pull request Nov 1, 2022

Migrate widgets to 2.x #344

Merged

6 tasks

unkcpz added 2 commits November 2, 2022 12:55

Merge branch 'master' into fix/uuid-as-traitlets-threading-widget

8a7fcfc

Merge branch 'master' into fix/uuid-as-traitlets-threading-widget

9a87416

danielhollas reviewed Nov 7, 2022

View reviewed changes

aiidalab_widgets_base/computational_resources.py Show resolved Hide resolved

danielhollas requested changes Nov 7, 2022

View reviewed changes

unkcpz and others added 2 commits November 7, 2022 16:51

r-dan

efbadae

[pre-commit.ci] auto fixes from pre-commit.com hooks

63000f4

for more information, see https://pre-commit.ci

unkcpz commented Nov 7, 2022

View reviewed changes

unkcpz requested a review from danielhollas November 7, 2022 16:54

danielhollas reviewed Nov 7, 2022

View reviewed changes

aiidalab_widgets_base/process.py Show resolved Hide resolved

process mornitor thread

5e23c7d

unkcpz requested a review from danielhollas November 10, 2022 16:17

Update aiidalab_widgets_base/process.py

13bacaa

Merge branch 'master' into fix/uuid-as-traitlets-threading-widget

6385f80

yakutovicha reviewed Nov 16, 2022

View reviewed changes

r-sa

04d47a1

unkcpz enabled auto-merge (squash) November 16, 2022 16:40

unkcpz disabled auto-merge November 16, 2022 16:52

unkcpz merged commit 2ec0db9 into master Nov 16, 2022

unkcpz deleted the fix/uuid-as-traitlets-threading-widget branch November 16, 2022 17:03

danielhollas mentioned this pull request Dec 4, 2022

computer is not a valid UUID #405

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UUID as traitlets for threading related widgets #375

UUID as traitlets for threading related widgets #375

unkcpz commented Oct 21, 2022 •

edited

Loading

danielhollas commented Oct 27, 2022

unkcpz commented Nov 1, 2022

danielhollas commented Nov 2, 2022

unkcpz commented Nov 7, 2022

danielhollas left a comment

danielhollas Nov 7, 2022

unkcpz Nov 7, 2022

danielhollas Nov 7, 2022

unkcpz Nov 7, 2022

unkcpz left a comment

unkcpz Nov 7, 2022

unkcpz Nov 7, 2022

unkcpz commented Nov 10, 2022

danielhollas commented Nov 10, 2022

unkcpz commented Nov 10, 2022

unkcpz commented Nov 16, 2022

yakutovicha left a comment

yakutovicha Nov 16, 2022

unkcpz Nov 16, 2022

danielhollas commented Nov 16, 2022

yakutovicha commented Nov 16, 2022 •

edited

Loading

-        if process_uuid is None or process_uuid != change["old"]:
+        if process_uuid == change["old"]:
+            return
+        # stop thread (if running)
+        if self._monitor_thread is not None:
+            with self._monitor_thread_lock:
+                self._monitor_thread_stop.set()
+                self._monitor_thread.join()
+        if process_uuid is None:
+            return
+        with self.hold_trait_notifications():
+            with self._monitor_thread_lock:
+                self._monitor_thread_stop.clear()
+                self._monitor_thread = Thread(
+                    target=self._monitor_process, args=(process_uuid,)
+                )
+                self._monitor_thread.start()

UUID as traitlets for threading related widgets #375

UUID as traitlets for threading related widgets #375

Conversation

unkcpz commented Oct 21, 2022 • edited Loading

danielhollas commented Oct 27, 2022

unkcpz commented Nov 1, 2022

danielhollas commented Nov 2, 2022

unkcpz commented Nov 7, 2022

danielhollas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz commented Nov 10, 2022

danielhollas commented Nov 10, 2022

unkcpz commented Nov 10, 2022

unkcpz commented Nov 16, 2022

yakutovicha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhollas commented Nov 16, 2022

yakutovicha commented Nov 16, 2022 • edited Loading

unkcpz commented Oct 21, 2022 •

edited

Loading

yakutovicha commented Nov 16, 2022 •

edited

Loading