aiidalab / aiidalab-widgets-base

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

Restore backwards compatibility for some widgets #471

Closed yakutovicha closed 1 year ago

yakutovicha commented 1 year ago

fixes #426

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 96.42% and project coverage change: +0.16 :tada:

Comparison is base (b99d88d) 79.32% compared to head (98717e5) 79.48%.

:exclamation: Current head 98717e5 differs from pull request most recent head f31b187. Consider uploading reports for the commit f31b187 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #471 +/- ## ========================================== + Coverage 79.32% 79.48% +0.16% ========================================== Files 27 27 Lines 3685 3709 +24 ========================================== + Hits 2923 2948 +25 + Misses 762 761 -1 ``` | Flag | Coverage Δ | | |---|---|---| | python-3.10 | `?` | | | python-3.8 | `79.48% <96.42%> (+0.12%)` | :arrow_up: | | python-3.9 | `79.48% <96.42%> (+0.12%)` | :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/471?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | Coverage Δ | | |---|---|---| | [aiidalab\_widgets\_base/process.py](https://codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/471?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-YWlpZGFsYWJfd2lkZ2V0c19iYXNlL3Byb2Nlc3MucHk=) | `78.96% <88.88%> (+0.37%)` | :arrow_up: | | [tests/test\_process.py](https://codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/471?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-dGVzdHMvdGVzdF9wcm9jZXNzLnB5) | `100.00% <100.00%> (ø)` | | ... and [2 files with indirect coverage changes](https://codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/471/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab)

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

danielhollas commented 1 year ago

@yakutovicha I will take a closer look at this, please wait before merging, thanks.

danielhollas commented 1 year ago

Although there is an issue caused by using threading with daemon as we discussed offline, I propose we ignore it for the moment.

@unkcpz can you elaborate on this? Is this problem present also on master or only on this PR? If the latter, we should not merge this. What aiida-core version have you seen it? I think there have been some issues with the daemon for version < 2.2.2

yakutovicha commented 1 year ago

tbh I am not very happy about the code here (and I realize I was the one pushing for this change, my apologies). The circular nature of the value and process traits makes the code very confusing and error prone imo. My original idea was to have a one-way flow from process to value, but that is not great either since they would not be in sync which might lead to bugs as well.

That is fine with me. But in that case, we must proceed with the backward incompatible changes listed here.

danielhollas commented 1 year ago

@yakutovicha that is fine with me, since you anyway said we have currently control over all apps in the appstore.

Btw: the new test in this PR might be worthwhile merging.

I am travelling today back from Switzerland, should be available more tomorrow.

unkcpz commented 1 year ago

@unkcpz can you elaborate on this? Is this problem present also on master or only on this PR? If the latter, we should not merge this. What aiida-core version have you seen it? I think there have been some issues with the daemon for version < 2.2.2

The error is shown in https://github.com/aiidalab/aiidalab-widgets-base/pull/471#pullrequestreview-1357821001 and can be reproduce with the latest docker stack which running with aiida-core 2.2.2.

It is not this PR which introduces the issue, it was there already. The tests will not failed but the error messages popped up showing the DB session is not properly shut down after the query. @yakutovicha and I suspect this might be the issue that causes the memory leak of the container we run for the moment.

unkcpz commented 1 year ago

Seems we don't want to continue on this route. So @yakutovicha can you update the PR to leave only the test as @danielhollas suggested?

yakutovicha commented 1 year ago

Seems we don't want to continue on this route. So @yakutovicha can you update the PR to leave only the test as @danielhollas suggested?

yep, I just opened PR #478 with the test only.