Control page: home page icon and panel placeholders - #232
Conversation
|
Thanks for working on this @yakutovicha! Left some comments, most notably regarding thread safety of the refresh method. |
70d805c to
b3b6174
Compare
It seems that the locking mechanism you propose makes things slightly more fragile. A few observations:
def refresh(self, _=None):
if self._refresh_lock.locked():
return
def worker():
with self._refresh_lock:
self._do_refresh()
threading.Thread(target=worker, daemon=True).start()
This leaves us with a possibility that two fast clicks can spawn 2 workers at the same time, which the current mechanism prevents entirely. |
edan-bainglass
left a comment
There was a problem hiding this comment.
Thanks @yakutovicha. LGTM w.r.t my comments.
danielhollas
left a comment
There was a problem hiding this comment.
Thanks @yakutovicha. I've found a couple other small things (some found by Claude), but happy for you to merge after addressing (or dismissing) them.
I'll reply to the threading issue separately.
|
|
||
|
|
||
| @pytest.fixture | ||
| def run_threads_synchronously(monkeypatch): |
There was a problem hiding this comment.
nit: This fixture feels very invasive, it would be nicer to write the tests in a robust way that would use an actual thread. But that can be done in a separate PR.
There was a problem hiding this comment.
Let's do it in a different PR, then. After this base PR is merged, many things can be implemented/discussed in parallel. Do you mind opening issues? 🙏
Sorry, my original suggestion indeed did not work, since the lock needs to be released from the worker thread. Here's a patch that I actually tested and seems to work. But I am happy to submit it as a separate PR since this would also require updating the tests and I don't want to block you. diff --git a/home/control.py b/home/control.py
index 19bd92e..7f9e756 100644
--- a/home/control.py
+++ b/home/control.py
@@ -28,7 +28,7 @@ class ControlSectionWidget(ipw.VBox):
description = ""
def __init__(self, children, show_refresh_button=True):
- self._refreshing = False
+ self._refresh_lock = threading.Lock()
header_children = []
if self.description:
@@ -70,9 +70,8 @@ class ControlSectionWidget(ipw.VBox):
self.info.value = html.escape(text)
def refresh(self, _=None):
- if self._refreshing:
+ if not self._refresh_lock.acquire(blocking=False):
return
- self._refreshing = True
if self.refresh_button is not None:
self.refresh_button.disabled = True
self.info.value = "Refreshing... <i class='fa fa-spinner fa-spin'></i>"
@@ -92,7 +91,7 @@ class ControlSectionWidget(ipw.VBox):
# button permanently disabled.
if self.refresh_button is not None:
self.refresh_button.disabled = False
- self._refreshing = False
+ self._refresh_lock.release()
if self._last_updated is not None:
self._last_updated.value = (
f"Last updated: {datetime.now().strftime('%H:%M:%S')}"
So I actually thought the the kernel uses a separate thread to handle UI interactions, but was apparently wrong about that (and verified by printing https://ianthomas23.github.io/jupytercon2025-subshells/#1 |
Co-authored-by: Daniel Hollas <daniel.hollas@bristol.ac.uk>
- always display full content of the page - prevent the page asking a confirmation before closing
fixes #155
Basic parts of the control section, extracted from #156.
Introduce a new "AiiDAlab control" page: a home-page entry point for
managing the AiiDA/AiiDAlab installation, with tab-based sections for
status, resources, storage, daemon, processes, profiles, and a danger
zone. Section bodies are placeholders in this PR; real logic for each
section lands in follow-up PRs.
start.py: add a "Control" icon tile linking tocontrol.ipynbcontrol.ipynb: new notebook entry point with a lazily-loaded tab persection; each tab probes fresh data both on first open and on every
revisit, and shows a retryable inline error if construction fails
home/control.py: addControlSectionWidget, the shared base class forevery section , plus placeholder subclasses for all seven sections
tests/test_control.py:cover refresh()'s re-entry guard, successpath, and error path