aiidalab / aiidalab-widgets-base

Reusable widgets for AiiDAlab applications
MIT License
6 stars 17 forks source link

Remove spinning Thread from WizzardAppWidgetStep #479

Closed danielhollas closed 1 year ago

danielhollas commented 1 year ago

WizzardAppWidget uses an infinite background thread to continously update titles of the Accordion widget. It looks like the only reason for this is to implement a dynamic "spinner". This thread just needlesly consumes server resources for a very little benefit. Implementing a "spinner" in this way is silly.

More importantly, it looks like this background thread is the main reason for the memory leak we're seeing, see https://github.com/aiidalab/issues/issues/13

Here I remove the thread in favour of a static unicode character, in line with all the other icons. It would be great if we could use some dynamic icons, e.g. fa-spinner as used in the SmilesWidget, but it looks like the Accordion.set_title() function does not parse HTML. Ultimately I think having static icon is fine, and users can implement something more dynamic in the child widgets.

TODO:

danielhollas commented 1 year ago

@unkcpz @yakutovicha I will be on holiday till Monday. Feel free to pick up this PR.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.10 :tada:

Comparison is base (9720334) 79.32% compared to head (a86e5f4) 79.42%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #479 +/- ## ========================================== + Coverage 79.32% 79.42% +0.10% ========================================== Files 27 27 Lines 3709 3728 +19 ========================================== + Hits 2942 2961 +19 Misses 767 767 ``` | Flag | Coverage Δ | | |---|---|---| | python-3.10 | `79.42% <100.00%> (+0.10%)` | :arrow_up: | | python-3.8 | `79.46% <100.00%> (+0.10%)` | :arrow_up: | | python-3.9 | `79.46% <100.00%> (+0.10%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/479?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | Coverage Δ | | |---|---|---| | [aiidalab\_widgets\_base/wizard.py](https://codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/479?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-YWlpZGFsYWJfd2lkZ2V0c19iYXNlL3dpemFyZC5weQ==) | `91.57% <100.00%> (-0.88%)` | :arrow_down: | | [tests/test\_wizard.py](https://codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/479?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-dGVzdHMvdGVzdF93aXphcmQucHk=) | `97.67% <100.00%> (+1.24%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

yakutovicha commented 1 year ago

@danielhollas, thanks a lot. I will take over implementing the unit test.

yakutovicha commented 1 year ago

[ ] Is there a better Unicode character that we could use?

I think the Unicode character you picked up ⌛ is good enough 👍

unkcpz commented 1 year ago

Super nice! thanks a lot, @danielhollas. I'll do the review after @yakutovicha implementation of the unit test.

yakutovicha commented 1 year ago

@unkcpz if you are ok with the current "busy" Unicode character, please tick the box. I am ok with it.

Otherwise, the PR is ready for your review.

danielhollas commented 1 year ago

Thank you @yakutovicha @unkcpz. Because the memory leak is such a critical issue, I would suggest to backport this to the 1.4 support branch.

unkcpz commented 1 year ago

I would suggest to backport this to the 1.4 support branch.

I'll take care of this after this one gets merged.