Use UUID rather than the aiida node instance as traitlet #354
Conversation
There was a problem hiding this comment.
I've started reviewing this and have a few suggestions and a few questions. I am a bit confused about how we use the validation mechanism here. Maybe we can clarify this before I move forward with further review.
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 ?
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.
|
@csadorf thanks for the review!
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. |
| # This place will never be reached, because the trait's type is checked. | ||
| return None | ||
| try: | ||
| _ = orm.load_code(code_uuid) |
There was a problem hiding this comment.
To my knowledge, the widget will be blocked while the validation is running. Therefore, I don't think we should run code as part of the validation that might require a significant amount of time (like a database query). I think the validation should not extend beyond very basic checks in the sense of whether the widget will enter a valid state with the provided value.
Anything beyond that should be handled as part of the business logic of the application. That means, we should inform consumers that the selected value does not correspond to a valid code and then allow those consumers to react to that in an appropriate manner.
There was a problem hiding this comment.
I see, thanks! The UUID is got from an instance queried from the database, to be honest, I see less point to check its existence.
Changed and please check the last commit.
| except ValueError: | ||
| self.output.value = f"""'<span style="color:red">{code_uuid}</span>' | ||
| is not a valid UUID.""" |
There was a problem hiding this comment.
I think the standard behavior is to raise a TraitError in this case, see here.
There was a problem hiding this comment.
No, this is the exception raised when the code_uuid is not a valid UUID. uuid.UUID module will raise a ValueError
For example:
from uuid import UUID
UUID("IAMNOTUUID", version=4)There was a problem hiding this comment.
Yes, but we should raise a TraitError in response.
Yes please 😊 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 🚀 |
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 😄 . |
There was a problem hiding this comment.
Just to mention that (apparently) an AiiDA object cannot be a trait and it is not 100% clear to me why. There are many widgets in this repository relying on AiiDA objects being traits.
I am still missing why the error pops up for the ComputationalResourcesWidget and doesn't manifest itself for the StructureManagerWidget (as we tested with @unkcpz).
Before approving this I would like to understand:
- Should we not use AiiDA objects as traits?
- If we shouldn't: why is that?
- If we still can, what are the new restrictions?
Can @chrisjsewell and/or @unkcpz comment on this? Understanding the points above can clarify how to move forward.
| """If code is provided, set it as it is. If code's UUID is provided, | ||
| check it is valid in DB""" |
There was a problem hiding this comment.
I am not sure this description is valid anymore. Looks like you forbid providing the code object (only ID or UUID).
|
Hi @yakutovicha, is this possible related to I make the above assumption since when testing the Let me know how to test this further. If |
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. |
|
As discussed offline, the issue reported in aiidalab/aiidalab-qe#279 is rooted in using unsafe multiple threads calls to access the AiiDA database which is not well handled by AiiDA at the moment. |
|
@unkcpz is it okay to delete the branch? |
|
Sure! Deleted. |
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 ?