Remove leftover load profile

[unkcpz]

The load_profile() in __init__.py to load the profile in module scope was introduced by PR #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.

• 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:

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

[codecov]

Codecov Report

Patch coverage: 82.75% and project coverage change: -0.27% ⚠️

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

Additional details and impacted files

@@            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	Coverage Δ
python-3.10	79.42% <82.75%> (-0.27%)	⬇️
python-3.8	79.46% <82.75%> (-0.27%)	⬇️
python-3.9	79.46% <82.75%> (-0.27%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
aiidalab_widgets_base/__init__.py	65.51% <44.44%> (-34.49%)	⬇️
tests/test_bug_report.py	100.00% <100.00%> (ø)
tests/test_computational_resources.py	100.00% <100.00%> (ø)
tests/test_elns.py	100.00% <100.00%> (ø)
tests/test_export.py	100.00% <100.00%> (ø)
tests/test_nodes.py	100.00% <100.00%> (ø)
tests/test_process.py	95.90% <100.00%> (ø)
tests/test_structures.py	100.00% <100.00%> (ø)
tests/test_viewers.py	100.00% <100.00%> (ø)
tests/test_wizard.py	97.67% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 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.

[danielhollas]

I like this change. In particular, this will allow in the future to support multiple AiiDA profiles.
However, as you note this will break most of the existing apps so we should not take this lightly. It's a pity that we did not do it before the 2.0 release. Should we wait with this for 3.0?

On a practical side, 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. This error should at least make it very clear to the developers what needs to be done.

[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 <#481 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACIY64PNXPSS74O76V7DABTXPZMCLANCNFSM6AAAAAAYCG3YSQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[yakutovicha]

Maybe just some nitpicking from my side, but otherwise good to go.

[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 ?

[unkcpz]

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

[danielhollas]

The warning message is very nice! Since it is rather intrusive, we should at least bump the minor version when we release this.

[danielhollas]

@unkcpz thanks, this looks great now. I've tested this locally. I only have one request for testing the new behaviour.

This should be released in 2.1 version. So before merging, we should release 2.0.2. There are still some tasks in that milestone. I've taken care of one in #502.

[danielhollas]

Can you please revert this change, so that we test that the default profile is loaded in init still?
Also, you could then extend the test_aiida_datatypes_viewers test in tests_notebooks/test_notebooks.py to check that the deprecation warning is displayed.

[unkcpz]

Good point, I'll try to add a test.

[yakutovicha]

LGTM,

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

%load_ext aiida
%aiida

[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 #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.

[yakutovicha]

Great work, @unkcpz!

[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.