aiidalab / aiidalab-widgets-base

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

Computer setup: Fix core.local transport #502

Closed danielhollas closed 11 months ago

danielhollas commented 1 year ago

It is not clear whether AiiDA Computer setup ever worked with any transport other than SSH, but in any case the widget is clearly designed with SSH in mind so unless a clear need is found we can simply remove the 'Transport type" dropdown. Note the the value of this widget has not been used anywhere in the code, which is telling.

The localhost computer is setup in the AiiDAlab Docker image by default, and in the unlikely case that another one needs to be setup, such user probably knows how to set it up via verdi command line interface.

Fixes #417

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 81.39% and project coverage change: +0.04% :tada:

Comparison is base (aec8f87) 79.46% compared to head (15494bf) 79.50%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #502 +/- ## ========================================== + Coverage 79.46% 79.50% +0.04% ========================================== Files 27 27 Lines 3759 3792 +33 ========================================== + Hits 2987 3015 +28 - Misses 772 777 +5 ``` | [Flag](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/502/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | Coverage Δ | | |---|---|---| | [python-3.10](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/502/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `79.50% <81.39%> (+0.04%)` | :arrow_up: | | [python-3.8](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/502/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `79.54% <81.39%> (+0.04%)` | :arrow_up: | | [python-3.9](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/502/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `79.54% <81.39%> (+0.04%)` | :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. | [Files Changed](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/502?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | Coverage Δ | | |---|---|---| | [aiidalab\_widgets\_base/computational\_resources.py](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/502?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-YWlpZGFsYWJfd2lkZ2V0c19iYXNlL2NvbXB1dGF0aW9uYWxfcmVzb3VyY2VzLnB5) | `70.76% <72.41%> (+0.10%)` | :arrow_up: | | [tests/test\_computational\_resources.py](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/502?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-dGVzdHMvdGVzdF9jb21wdXRhdGlvbmFsX3Jlc291cmNlcy5weQ==) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

danielhollas commented 12 months ago

However, later I am introducing the https://github.com/aiidateam/aiida-core/pull/6083 which will revert things to Dropdown. Then, there will be firecrest. It would take some more work to adopt them, though.

Ah, that's good to know. I also found out that the transport value is indeed passed to the computer setup function, I did not notice that before since it is accessed via getattrs

https://github.com/aiidalab/aiidalab-widgets-base/blob/413c26c76ec438b9c70afe3437420923f738e50b/aiidalab_widgets_base/computational_resources.py#L898

I would only request to keep the self.transport.value = "core.ssh" line and set the original value to core.ssh too. It was issued later on. Also, settings downloaded from the code database override the value. Herein, SSH as the value may cause trouble.

Another good point. As a side note, I am not a fan of this design, core.ssh is not very user friendly, we're essentially exposing the user to internal AiiDA details. But that's for another time...

Given all of the above, I've reverted the original change and instead fix the underlying bug and made the core.local transport work. I also added a unit test. @yakutovicha could you please test locally?

danielhollas commented 11 months ago

Great, thanks @yakutovicha!