Implement and utilize countdown widget (for temporary deployments) #198
Conversation
|
@superstar54 @mikibonacci can I get a review please? 🙏 Pinging @danielhollas also if he has time 🙂 |
There was a problem hiding this comment.
Hi @edan-bainglass , I missed the previous review request since I get flooded with hundreds of GitHub (action) emails daily. 🤣 .
See my comments below.
| "\n", | ||
| "# For temporary deployments (e.g. demo server), duration is read from a local\n", | ||
| "# config file. This will initiate the countdown widget.\n", | ||
| "demo_server_config_file = Path.home() / \".aiidalab\" / \"demo-server-config.yml\"\n", |
There was a problem hiding this comment.
Similar to the QEapp, we don't want to hardcode the demo-server in the app, so I would suggest using aiidalab-home.yaml, or aiidalab.yaml directly.
There was a problem hiding this comment.
Actually, this is explicitly for demo servers (not our demo server, but for demo servers)
start.ipynb
Outdated
| "demo_server_config_file = Path.home() / \".aiidalab\" / \"demo-server-config.yml\"\n", | ||
| "if demo_server_config_file.exists():\n", | ||
| " demo_server_config = safe_load(demo_server_config_file.read_text())\n", | ||
| " countdown = CountDownWidget(duration=demo_server_config.get(\"duration\", \"12:00:00\"))\n", |
There was a problem hiding this comment.
- change
durationto a better name, e.g.,container_lifetime - if the lifetime does not set explicitly in the config file, the default value should be a very long time (e.g., 10 years), instead of
12:00:00.
There was a problem hiding this comment.
Actually, if the duration is not set in the config file, there should be no countdown widget deployed. I will adjust the conditional.
| if len(elapsed_str) < 6: | ||
| elapsed_str = f"00:{elapsed_str}" | ||
| elapsed_time = datetime.strptime(elapsed_str, "%H:%M:%S") |
There was a problem hiding this comment.
Also consider time longer than one day, (maybe even months).
There was a problem hiding this comment.
What use case are you thinking of here? I'm not trying to make this general. This is specifically for demo server deployments.
There was a problem hiding this comment.
I understand that our current demo server only supports 12 hours, but since this is in the AiiDAlab-home, it should be more general, because other people may use this feature, or, we may use this feature in another deployment longer than 12 hours.
There was a problem hiding this comment.
We can update this for "other people" down the road. For now, this is fine for our use case.
There was a problem hiding this comment.
Just more caution for the aiidalab-home because every time we make a new release, the docker image must be upgraded.
But OK to me if you intend to leave this for the future.
|
@edan-bainglass can you provide screenshots of how this looks? Is the countdown displayed only in the home page? |
Updated description with images. And yes, only on the home page. |
Hi @danielhollas. Did you have a question or comment regarding this PR? |
|
@edan-bainglass appologies for late answer, I've been moving to a new flat today so last week has been busy. :-)
My main concern here is that this doesn't feel like something that should live in For example, you could have this as part of the QeApp's |
This would limit the countdown to the QE App home app container. If for some reason the demo server user uninstalls the QE App (unlikely), no more countdown. I understand that the countdown widget needs work to make it available for more use cases. However, I fail to see how this is an issue at the present stage. There have not been any use cases other than the demo server, so no hypothetical use cases will miss the fact that it is not yet general. Placing it here will immediately make it useful to its first use case. When time permits, we iterate, generalize, and release a patch or minor release. Please explain the practical reasons why you believe this shouldn't be accepted. I'm happy to change my opinion if I'm overlooking something. |
While you're at it, good if you can bullet point the missing features required in your opinion for this to be accepted here. I can then better assess if it is doable in a reasonable time frame 🙂 |
The issue is that if you put this here, that whenever you need to change it, you need to:
That's a lot of churn for something that you currently need only for the demo server. Also any code that's only for Demo server always risks breaking something for the other use cases (I am not saying this is the case here, just as a general principle). If you don't want to tie it to QeApp, you could also just inject it as its own little app, i.e. put it inside a folder in In any case, I just wanted to raise this concern, I don't want to block anything. But that is another point, if the demo server stuff is localized to QeApp, then the review also doesn't need to be so strict since you can iterate faster. 🤷 :-) |
I think the main thing is that I would put this in the top header (perhaps next to the AiiDAlab app logo) since then it would be always displayed. If the user open QeApp, then it's going to be very easy to miss the countdown. I also think that the timer should be implemented on the JS side, the only thing that it needs from the backend is the start time, right? Right now you're executing subprocess call every second in a background thread, and change the HTML output every second, if I understand correctly. That's a lot of unnecessary traffic. More importantly, this can lead to a severe memory leak if the user closes the browser window unexpectedly (such putting his computer to sleep). We've had huge issues with this in the past, see: https://github.com/aiidalab/issues/issues/13#issuecomment-1523233470 |
There was a problem hiding this comment.
Disregarding the philosophical issues discussed above, I am afraid this has a fundamental flaw of running infinite background thread that can cause memory leaks. Please have a look at https://github.com/aiidalab/issues/issues/13, I am happy to explain more.
| ) | ||
|
|
||
| def start(self): | ||
| Timer(1, self._update_countdown).start() |
There was a problem hiding this comment.
Infinitely running background threads that update the UI state are unfortunately a really bad idea due to a possible memory leak described in https://github.com/aiidalab/issues/issues/13.
I am afraid timer like this needs to be in javascript. If I am not mistaken, as long as the timer is initialized properly, it shouldn't need any information from the backend, right?
|
Thank you @danielhollas. This is very clear. I will rethink my solution. |
This PR implement a count down widget for use in temporary deployments. The widget is added to the main notebook within a check for a demo-server-config.yml file.
To test, add
~/.aiidalab/demo-server-config.ymlwithduration: "##:##:##"in it. Try different times. In particular, try near 5 minutes to check the color change, and near 0 to see the final message.Checking elapsed time is done via
ps -o etime= -p 1, as suggested by @yakutovicha