search

aiidalab / aiidalab-widgets-base

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

Use UUID rather than the aiida node instance as traitlet #354

Closed unkcpz closed 2 years ago

unkcpz commented 2 years ago

From AiiDA 2.x every orm entity instance is now tied to a specific storage instance (which is connected to the database). So once the storage instance is closed (and so close the connection), then those orm instances are now "dead" and can not be used for other reloaded sessions.

Since this requires changing the API of the ComputationalResourceWidget, I create this dedicated PR for it. Do we also consider the back-ward compatibility or do we just leave this all to the 2.x support by put all these changes to #344 ?

unkcpz commented 2 years ago

@csadorf thanks for the review!

I think we established that AWB that supports AiiDA 2.x will not support 1.x. In so far, this change set does not need to be backwards-compatible.

I agree. But this means we need to care about the release. I suggest cherry pick and release a version without this feature for 1.x supported only and after that having this new API a new version released with PR #344.

danielhollas commented 2 years ago

I agree. But this means we need to care about the release. I suggest cherry pick and release a version without this feature for 1.x supported only and after that having this new API a new version released with PR https://github.com/aiidalab/aiidalab-widgets-base/pull/344.

Yes please :blush: I asked @yakutovicha to cut a new 1.x release now to release my recent PRs so you might want to hold on merging this till then maybe (or have a separate 2.x branch). EDIT: Release made :rocket:

yakutovicha commented 2 years ago

I agree. But this means we need to care about the release. I suggest cherry pick and release a version without this feature for 1.x supported only and after that having this new API a new version released with PR #344.

As a side note: I just created support/aiida-1.x branch. All the bug fixes for the aiida-1.x should go there. You can now feel free to brake things in the master branch 😄 .

unkcpz commented 2 years ago

Hi @yakutovicha, is this possible related to self.hold_trait_notifications()? I don't know how this contextmanager work under the hood, but from its functionality, it will run the code (where inside refresh function it will query for the aiida data for the code/computer node) and hold the notification. I guess the issue comes from where calls the update of this widget is a new session and the error pop up.

I make the above assumption since when testing the ComputationalResourcesWidget itself from the notebook all works fine, the issue occurs only when embedding the widget into qeapp.

Let me know how to test this further. If hold_trait_notifications is the catch, I'd say we should not use AiiDA objects as traits in all cases.

yakutovicha commented 2 years ago

Let me know how to test this further. If hold_trait_notifications is the catch, I'd say we should not use AiiDA objects as traits in all cases.

I don't know how to test it at the moment. I need to look closer. But I would very much prefer to keep using AiiDA objects as traits, cause otherwise, we will need a huge rewrite of the AWB library.

unkcpz commented 2 years ago

As discussed offline, the issue reported in https://github.com/aiidalab/aiidalab-qe/issues/279 is rooted in using unsafe multiple threads calls to access the AiiDA database which is not well handled by AiiDA at the moment. I'll close this PR and fix the thread issue in QeApp.

yakutovicha commented 2 years ago

@unkcpz is it okay to delete the branch?

unkcpz commented 2 years ago

Sure! Deleted.