Remove process notebooks #720
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #720 +/- ##
=======================================
Coverage 84.27% 84.27%
=======================================
Files 18 18
Lines 3581 3581
=======================================
Hits 3018 3018
Misses 563 563
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
danielhollas
left a comment
danielhollas
left a comment
There was a problem hiding this comment.
Thanks, code looks good, but please don't merge yet before we release 2.6 and before we have a chance to discuss a bit more about the coupling between AWB and home app (which I think we should avoid if possible, let's discuss over at aiidalab-home repo)
| if self.append_output: | ||
| self.submit_out.value += f"""Submitted process {self.process}. Click | ||
| <a href={self.path_to_root}aiidalab-widgets-base/notebooks/process.ipynb?id={self.process.pk} | ||
| <a href={self.path_to_root}home/process.ipynb?id={self.process.pk} |
There was a problem hiding this comment.
This new coupling the the home app is unfortunate :-( Seems very brittle.
There was a problem hiding this comment.
Thinking about this a bit more, I feel somewhat strongly that we should not link to the home app like this from individual widgets.
At least for the case of SubmitButtonWidget, should we just remove this link altogether? @edan-bainglass I am not sure if QeApp depends on this widget or if it has its own thing?
There was a problem hiding this comment.
We do not use this widget on the app. We have our own submission logic. I'm not familiar with this widget, but indeed, AWB widgets probably shouldn't have hard links. Instead, if we want to keep a link feature (not a bad idea), the link should be injected into the widget.
Yes, made an issue aiidalab/aiidalab-home#212, please share your arguments. |
danielhollas
left a comment
danielhollas
left a comment
There was a problem hiding this comment.
Some more thoughts about the links to the notebooks moved to home app.
This can also be deferred for future PR, but we should definitely think about this a bit more before the final AWB 3.0 release (e.g. do we want to perhaps get rid of some of the widgets if they are gonna live in the home app?).
| if self.append_output: | ||
| self.submit_out.value += f"""Submitted process {self.process}. Click | ||
| <a href={self.path_to_root}aiidalab-widgets-base/notebooks/process.ipynb?id={self.process.pk} | ||
| <a href={self.path_to_root}home/process.ipynb?id={self.process.pk} |
There was a problem hiding this comment.
Thinking about this a bit more, I feel somewhat strongly that we should not link to the home app like this from individual widgets.
At least for the case of SubmitButtonWidget, should we just remove this link altogether? @edan-bainglass I am not sure if QeApp depends on this widget or if it has its own thing?
|
|
||
| process_state = node.process_state.value.capitalize() | ||
| pk = f"""<a#space#href={self.path_to_root}aiidalab-widgets-base/notebooks/process.ipynb?id={node.pk}#space#target="_blank">{node.pk}</a>""" | ||
| pk = f"""<a#space#href={self.path_to_root}home/process.ipynb?id={node.pk}#space#target="_blank">{node.pk}</a>""" |
There was a problem hiding this comment.
Here again, should we just remove the link?
There was a problem hiding this comment.
Let the client inject a link on widget instantiation if they wish.
| # Add HTML links. | ||
| dataf["PK"] = dataf["PK"].apply( | ||
| lambda x: f"""<a href={self.path_to_root}aiidalab-widgets-base/notebooks/process.ipynb?id={x} target="_blank">{x}</a>""" | ||
| lambda x: f"""<a href={self.path_to_root}home/process.ipynb?id={x} target="_blank">{x}</a>""" |
There was a problem hiding this comment.
This is in ProcessList widget. Crazy idea, but since we're bumping major version, do we even want ProcessList widget as part of AWB, if we make its functionality part of home app? (that would partly of "resolve" the vendoring problem since the code would just live in the home app.
There was a problem hiding this comment.
I'm not sure where and how we use the widget, but in principle, I would say all these core AiiDA operations will be provided via the AiiDA GUI in JS. I foresee this integrated by March!
edan-bainglass
left a comment
edan-bainglass
left a comment
There was a problem hiding this comment.
Thanks @yakutovicha . LGTM! Regarding the links, let's allow the user to inject them via widget constructors as to avoid hard wiring them, to home app or any other place. Happy to discuss 🙂
…/aiidalab-widgets-base into update/remove-notebooks
fixes #719
related to aiidalab/aiidalab-home#211