Remove leftover load profile

[unkcpz]

The load_profile() in __init__.py to load the profile in module scope was introduced by PR https://github.com/aiidalab/aiidalab-widgets-base/pull/12. It is not needed anymore since the computer and code widgets are updated and can update the dropdown dynamically.

More problematically that this makes the profile loaded in the unit tests and overrides the test profile which makes it has to put the module load inside every test. There is also the problem that if the user want to test the widgets in a newly create python environment that doesn't have an AiiDA profile setup impossible.

• [x] move modules import from test to file in front.

Before meeting this, we should make sure that apps which depend on AWB, load profiles in the notebooks. We checkbox the apps if they do:

• [x] Aurora app (doesn't depend on AWB)
• [ ] Molecules app
• [x] LSMO app (not migrated)
• [ ] Martket place - use case 3
• [x] Graphene nanoribbons
• [ ] Quantum ESPRESSO app
• [x] Scanning Probe Microscopy (deprecated)
• [ ] SSSP toolbox
• [x] On-Surface Chemistry

[codecov[bot]]

Codecov Report

Patch coverage: 82.75% and project coverage change: -0.27% :warning:

Comparison is base (a6fcf06) 79.69% compared to head (2db5584) 79.42%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #481 +/- ## ========================================== - Coverage 79.69% 79.42% -0.27% ========================================== Files 27 27 Lines 3757 3757 ========================================== - Hits 2994 2984 -10 - Misses 763 773 +10 ``` | [Flag](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/481/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/481/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `79.42% <82.75%> (-0.27%)` | :arrow_down: | | [python-3.8](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/481/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `79.46% <82.75%> (-0.27%)` | :arrow_down: | | [python-3.9](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/481/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `79.46% <82.75%> (-0.27%)` | :arrow_down: | 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/481?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | Coverage Δ | | |---|---|---| | [aiidalab\_widgets\_base/\_\_init\_\_.py](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/481?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-YWlpZGFsYWJfd2lkZ2V0c19iYXNlL19faW5pdF9fLnB5) | `65.51% <44.44%> (-34.49%)` | :arrow_down: | | [tests/test\_bug\_report.py](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/481?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-dGVzdHMvdGVzdF9idWdfcmVwb3J0LnB5) | `100.00% <100.00%> (ø)` | | | [tests/test\_computational\_resources.py](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/481?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%> (ø)` | | | [tests/test\_elns.py](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/481?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-dGVzdHMvdGVzdF9lbG5zLnB5) | `100.00% <100.00%> (ø)` | | | [tests/test\_export.py](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/481?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-dGVzdHMvdGVzdF9leHBvcnQucHk=) | `100.00% <100.00%> (ø)` | | | [tests/test\_nodes.py](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/481?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-dGVzdHMvdGVzdF9ub2Rlcy5weQ==) | `100.00% <100.00%> (ø)` | | | [tests/test\_process.py](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/481?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-dGVzdHMvdGVzdF9wcm9jZXNzLnB5) | `95.90% <100.00%> (ø)` | | | [tests/test\_structures.py](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/481?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-dGVzdHMvdGVzdF9zdHJ1Y3R1cmVzLnB5) | `100.00% <100.00%> (ø)` | | | [tests/test\_viewers.py](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/481?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-dGVzdHMvdGVzdF92aWV3ZXJzLnB5) | `100.00% <100.00%> (ø)` | | | [tests/test\_wizard.py](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/481?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%> (ø)` | |

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

[unkcpz]

well, there is one thing needs to change for the existing notebooks or apps the load_profile need to be explicitly called from app. Which also means the apps need to be adopted. I think it is a more proper way to do it, as instructed in the aiida doc https://aiida.readthedocs.io/projects/aiida-core/en/latest/intro/installation.html?highlight=jupyter%20notebook#using-aiida-in-jupyter.

pinning @yakutovicha @danielhollas for comments.

[unkcpz]

Should we wait with this for 3.0?

I don't think this will happen soon. @yakutovicha we still have control of most of app, do you think it is doable to make this change? I'd argue this can be a feature that is worth adding with a minor release.

I would propose to add a code in aiidalab_widgets_base/init.py that would check whether the AiiDA profile is loaded and throws a RuntimeError if that is not the case.

I didn't see the point of doing this. This is to prevent the loaded profile being overrided by the load_profile of aiidalab_widgets_base/__init__.py? Changing/overriding the profile in a notebook is not a problem I guess?

[danielhollas]

I didn't see the point of doing this. This is to prevent the loaded profile being overrided by the load_profile of aiidalab_widgets_base/init.py?

The point is only to provide a clear error message in case the developer forgets to call load_profile() before loading AWB. I do not know what happens now, perhaps the existing error that one gets is clear enough?

The question I had not considered is whether there are widgets which do not require loaded AiiDA profile? In which case we should not have this runtime check I guess.

[yakutovicha]

I don't think this will happen soon. @yakutovicha we still have control of most of app, do you think it is doable to make this change? I'd argue this can be a feature that is worth adding with a minor release.

I don't think it is a big problem to do that. In most of our notebooks, we have %aiida magic which does that already.

At the moment, the only fear I have is that some of the AWB modules that require AiiDA classes will fail to load. Maybe this is ok (in the end, for exactly that reason we launch verdi shell instead of just ipython).

[yakutovicha]

So I would probably go with this PR taking the suggestion of @danielhollas to through RuntimeError in case the profile is not loaded.

[unkcpz]

load_profile() before loading AWB. I do not know what happens now, perhaps the existing error that one gets is clear enough?

As I did try in lastest two commits, if the check is put in the __init__.py it will run when the module is imported, and the exception will then raise if the profile is not loaded before import aiidalab_widgets_base. I think the current error is clear enough for AiiDA users. Therefore I rollback the change.

[yakutovicha]

@unkcpz, is there anything left here to be done?

[danielhollas]

Are we in a rush for this? This is a hard breaking change for essentially all apps that use AWB.

Dne st 12. 7. 2023 9:13 uživatel Aliaksandr Yakutovich < @.***> napsal:

@unkcpz https://github.com/unkcpz, is there anything left here to be done?

— Reply to this email directly, view it on GitHub https://github.com/aiidalab/aiidalab-widgets-base/pull/481#issuecomment-1632051253, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACIY64PNXPSS74O76V7DABTXPZMCLANCNFSM6AAAAAAYCG3YSQ . You are receiving this because you were mentioned.Message ID: @.***>

[unkcpz]

Are we in a rush for this? This is a hard breaking change for essentially

Not in a rush and yes it is a change that will break other apps. So I tag it to milestone 2.1.0. But it needs to happen sometime, so the question to me is what is the blocker for not making this move? At least we still have all control of all apps and we are in the stage of migrating things, once all app migration in EMPA is finished or your app is released, there will be more resistance to pushing breaking changes, I guess?

@unkcpz, is there anything left here to be done?

Yep, It is ready. I'll address your requests.

[yakutovicha]

Are we in a rush for this? This is a hard breaking change for essentially all apps that use AWB.

I didn't mean to rush anybody.

I saw that @unkcpz put the 2.1.0 milestone, which is very soon, and my question is if I rephrase "what should be done to merge it?"

Do we need to do some preparatory work to make sure all apps have profiles loaded? We can, for example, list all the apps and tick-box them if they do.

[unkcpz]

We can, for example, list all the apps and tick-box them if they do.

This is a good idea, can you create a list of apps and tick boxes for EPMA apps? I'll check other apps.

[yakutovicha]

We can, for example, list all the apps and tick-box them if they do.

This is a good idea, can you create a list of apps and tick boxes for EPMA apps? I'll check other apps.

Sure, I will put it in the top comment.

[danielhollas]

Not in a rush and yes it is a change that will break other apps. So I tag it to milestone 2.1.0. But it needs to happen sometime, so the question to me is what is the blocker for not making this move? At least we still have all control of all apps and we are in the stage of migrating things, once all app migration in EMPA is finished or your app is released, there will be more resistance to pushing breaking changes, I guess?

Note that I've already sent my app to some external collaborators so I'd really like things to be stable. At the very least, for breaking things like this I'd like us to get back into a habit of having a reasonable deprecation period.

The good news here is that we can first make all the existing apps ready for this change, by making sure they load the profile in the notebook (loading profile twice is okay).

I would propose to first do:

1. In AWB __init__, check whether profile is loaded. If it is, do nothing.
2. If a profile is not loaded, load the default profile and emit a deprecation warning.

In this way, we can release this change in a fully backwards compatible manner (even in a patch release). At the same time it would already allow us to do all the things we wanted (e.g. load non-default profiles, easier unit testing).

WDYT @unkcpz @yakutovicha ?

[yakutovicha]

I would propose to first do:

1. In AWB __init__, check whether profile is loaded. If it is, do nothing.
2. If a profile is not loaded, load the default profile and emit a deprecation warning.

@unkcpz I assume you take care of that, right?

[unkcpz]

Yes, I'll take care of this one.

[danielhollas]

@unkcpz thanks! Since this is going to be backward compatible for now, I've moved it to 2.0.2 release. LMK in case you disagree.

[unkcpz]

The PR is updated and ready to review @danielhollas @yakutovicha , the warning looks as below:

[unkcpz]

Hold on, the unit tests failed because for the test profile the load_profile is still called. Ready ;)

[unkcpz]

We have already discussed this - let's try to avoid doing compound PRs. It's better to do one thing within one PR. If you don't mind - please make a separate PR with this change only. It will be approved immediately.

Yes, I move it to https://github.com/aiidalab/aiidalab-widgets-base/pull/503

[unkcpz]

The only thing I would consider is whether to propose to the developers an alternative solution using ipython magic:

%load_ext aiida
%aiida

Yes, we can, but I remember there are issues with this magic, although all are fixed (https://github.com/search?q=repo%3Aaiidateam%2Faiida-core+ipython+magic&type=issues) but it is less robust than the load_profile I'd say. So I would recommend using the load_profile.

[unkcpz]

Thanks for the review! This will go to v2.1 so anyone who makes the release should remember to cherry-pick commits when doing the release.