aiidalab / aiidalab-widgets-base

Reusable widgets for AiiDAlab applications
MIT License
7 stars 17 forks source link

UUID as traitlets for threading related widgets #375

Closed unkcpz closed 1 year ago

unkcpz commented 2 years ago

The widget changed are:

danielhollas commented 2 years ago

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

unkcpz commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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.

unkcpz commented 1 year ago

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

danielhollas commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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.

danielhollas commented 1 year ago

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 commented 1 year ago

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 👍