Consolidate AWB preview handling - #220
Conversation
AWB is still called in some places, but only to display widgets. In this PR we make sure that AWB is only called when it is installed. Otherwise, nothing is shown and a helper message is given to the user.
danielhollas
left a comment
danielhollas
left a comment
There was a problem hiding this comment.
As an alternative, instead of displaying the message, you could display a fallback view of the node with just it's __repr__ or __str__, i.e. the same thing that the AWB viewer does for nodes that don't have dedicated viewer.
(presumably, if the plan still to pre-install an AWB version to the image in the future, this fallback should be rare)
danielhollas
left a comment
danielhollas
left a comment
There was a problem hiding this comment.
@yakutovicha can we get this one merged? Then we can make a aiidalab-home release.
Co-authored-by: Daniel Hollas <daniel.hollas@bristol.ac.uk>
| try: | ||
| from aiidalab_widgets_base import viewer # noqa: PLC0415 | ||
| except ImportError: | ||
| return AWB_UNAVAILABLE_MESSAGE |
There was a problem hiding this comment.
My suggestion here was to return the node instead of the AWB_UNAVAILABLE_MESSAGE to simplify things, since AWB should anyway be present in typical case. @yakutovicha If you disagree can you explain why?
EDIT: Sorry, just saw your message on Slack, I'll defer to Edan what he thinks.
There was a problem hiding this comment.
Unclear how "since AWB should anyway be present in typical case" justifies "return the node instead of the AWB_UNAVAILABLE_MESSAGE" 🤔 Can you clarify?
In any case, I would do here what is best for the user. If we return the node without an available viewer, the user would just see the repr of the node, right? And no clear message as to why they don't see the viewer. Is this useful to the common user? It might be sufficient to some to see the node repr message, but I would still notify them somehow that there's more to see if they install AWB.
There was a problem hiding this comment.
Yeah, that looks good to me, thanks! Maybe wrap the command in <pre> tags.
There was a problem hiding this comment.
"For a richer node view, please install the AiiDAlab widgets with pip install aiidalab-widgets-base"
Not sure if "node view" is clear to the common user. Maybe "output view" is better.
There was a problem hiding this comment.
Yeah, that looks good to me, thanks! Maybe wrap the command in
\<pre\>tags.
<pre> puts it on a separate line, so I used <code>.
here 91e78a7.
There was a problem hiding this comment.
"For a richer node view, please install the AiiDAlab widgets with
pip install aiidalab-widgets-base"Not sure if "node view" is clear to the common user. Maybe "output view" is better.
done in 3165788.
edan-bainglass
left a comment
edan-bainglass
left a comment
There was a problem hiding this comment.
I'm a bit unclear on the goal of this change. AWB import was already guarded with a try/except. This PR appears to simply consolidate it (which is fine/cleaner). But then I would state that more clearly, rather than stating that THIS PR makes AWB a soft dependency. Feels like I'm missing something 😅
Fair point, it is indeed more about consolidation. I am going to rename the PR. |
yakutovicha
changed the title
edan-bainglass
left a comment
edan-bainglass
left a comment
There was a problem hiding this comment.
Thanks @yakutovicha. LGTM!
|
@danielhollas could you also approve this one please? 🙏 |
|
Yrp, will do do after lunch |
danielhollas
left a comment
danielhollas
left a comment
There was a problem hiding this comment.
Thanks @yakutovicha!
Co-authored-by: Daniel Hollas <daniel.hollas@bristol.ac.uk>
for more information, see https://pre-commit.ci
3 participants
AWB is still called in some places, but only to display widgets. In this PR we make sure that AWB is only called when it is installed. Otherwise, nothing is shown and a helper message is given to the user.