Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix aiida.cmdline.utils.decorators.load_backend_if_not_loaded #4878

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Apr 28, 2021

Fixes #4877

The function incorrectly used the PROFILE global variable to determine
whether the database backend environment had been loaded. This variable
is set as soon as a profile is loaded, however, this does not
automatically mean that the database environment is also loaded. These
two actions are separate on purpose such that a profile can be loaded
without having to load the backend, since that is an expensive operation
and is not always necessary.

This bug meant that sometimes the backend would actually not be loaded
even though the with_dbenv decorator was correctly used. This affects,
among other things, all CLI commands that rely on the backend being
loaded but having no code in its execution path that will automatically
load the backend, such as verdi archive import. Any commands that deal
with the ORM still work, since loading any ORM entity will automatically
load the backend of the current profile if not already done. Since the
import functionality circumvents the ORM, it didn't have this failsafe
operation.

Finally, the bug was not noticed by the unittests because they run in an
environment where the database backend is loaded anyway for the test
profile effectively hiding the bug of the load_backend_if_not_loaded
method.

@sphuber sphuber requested a review from chrisjsewell April 28, 2021 13:50
@sphuber
Copy link
Contributor Author

sphuber commented Apr 28, 2021

The question now is how we test that nothing else is broken 🤔

@chrisjsewell
Copy link
Member

The question now is how we test that nothing else is broken

yeh thats I was gonna say lol; are you 100%, totally and utterly sure this is fully fixed 😬
Feels like this should be calling a function/method, rather than checking a global variable?

@sphuber
Copy link
Contributor Author

sphuber commented Apr 28, 2021

yeh thats I was gonna say lol; are you 100%, totally and utterly sure this is fully fixed

No. It is clear that our unit tests would not catch this problem. I am pretty sure about the analysis as to the cause and that should be fixed, but there is no telling if there is anything else broken that is being hidden.

Feels like this should be calling a function/method, rather than checking a global variable?

Can refactor this into two utility methods is_profile_loaded and is_backend_loaded.

@chrisjsewell
Copy link
Member

chrisjsewell commented Apr 28, 2021

I have to say, I don't really get the purpose of #4865 in the first place; since get_manager().get_backend() already only loads the backend if its not loaded, so there is no extra cost there.
Is it just because you can skip the spinner creation?

@chrisjsewell
Copy link
Member

oh and there is also, already the Manager.backend_loaded property

@codecov
Copy link

codecov bot commented Apr 28, 2021

Codecov Report

Merging #4878 (5c15aca) into develop (16b326b) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4878      +/-   ##
===========================================
+ Coverage    79.67%   79.68%   +0.01%     
===========================================
  Files          523      523              
  Lines        37168    37169       +1     
===========================================
+ Hits         29610    29614       +4     
+ Misses        7558     7555       -3     
Flag Coverage Δ
django 74.27% <100.00%> (+0.04%) ⬆️
sqlalchemy 73.20% <100.00%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
aiida/cmdline/utils/decorators.py 84.22% <100.00%> (+5.64%) ⬆️
aiida/engine/daemon/client.py 75.13% <0.00%> (-1.01%) ⬇️
aiida/transports/plugins/local.py 81.80% <0.00%> (+0.26%) ⬆️
aiida/manage/manager.py 81.36% <0.00%> (+0.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16b326b...5c15aca. Read the comment docs.

@sphuber
Copy link
Contributor Author

sphuber commented Apr 28, 2021

You are right, the fix did prevent the spinner from unnecessarily appearing but get_backend() would have guarded already against actual multiple loadings of the backend. Will improve the code

@sphuber sphuber force-pushed the fix/4877/load-backend-if-not-loaded branch from 76e4610 to e799862 Compare April 28, 2021 14:24
manager = get_manager()

if not manager.backend_loaded:
with spinner():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, do we really have to load two spinners now.
Maybe just do if get_profile() is None or not manager.backend_loaded

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and then always call load_profile relying on its internal shortcut to not load again if already loaded?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeh that better cheers

The function incorrectly used the `PROFILE` global variable to determine
whether the database backend environment had been loaded. This variable
is set as soon as a profile is loaded, however, this does not
automatically mean that the database environment is also loaded. These
two actions are separate on purpose such that a profile can be loaded
without having to load the backend, since that is an expensive operation
and is not always necessary.

This bug meant that sometimes the backend would actually not be loaded
even though the `with_dbenv` decorator was correctly used. This affects,
among other things, all CLI commands that rely on the backend being
loaded but having no code in its execution path that will automatically
load the backend, such as `verdi archive import`. Any commands that deal
with the ORM still work, since loading any ORM entity will automatically
load the backend of the current profile if not already done. Since the
import functionality circumvents the ORM, it didn't have this failsafe
operation.

Finally, the bug was not noticed by the unittests because they run in an
environment where the database backend is loaded anyway for the test
profile effectively hiding the bug of the `load_backend_if_not_loaded`
method.
@sphuber sphuber force-pushed the fix/4877/load-backend-if-not-loaded branch from e799862 to 5c15aca Compare April 28, 2021 14:50
@sphuber sphuber requested a review from chrisjsewell April 28, 2021 14:50
@sphuber sphuber merged commit e335e30 into aiidateam:develop Apr 28, 2021
@sphuber sphuber deleted the fix/4877/load-backend-if-not-loaded branch April 28, 2021 15:11
sphuber added a commit that referenced this pull request Apr 28, 2021
The function incorrectly used the `PROFILE` global variable to determine
whether the database backend environment had been loaded. This variable
is set as soon as a profile is loaded, however, this does not
automatically mean that the database environment is also loaded. These
two actions are separate on purpose such that a profile can be loaded
without having to load the backend, since that is an expensive operation
and is not always necessary.

This bug meant that sometimes the backend would actually not be loaded
even though the `with_dbenv` decorator was correctly used. This affects,
among other things, all CLI commands that rely on the backend being
loaded but having no code in its execution path that will automatically
load the backend, such as `verdi archive import`. Any commands that deal
with the ORM still work, since loading any ORM entity will automatically
load the backend of the current profile if not already done. Since the
import functionality circumvents the ORM, it didn't have this failsafe
operation.

Finally, the bug was not noticed by the unittests because they run in an
environment where the database backend is loaded anyway for the test
profile effectively hiding the bug of the `load_backend_if_not_loaded`
method.

Cherry-pick: e335e30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants