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.with_dbenv always loading the db #4865

Merged
merged 2 commits into from
Apr 26, 2021

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Apr 21, 2021

Fixes #4864

The premise of the method with_dbenv is that it will load the profile
and corresponding database environment if not already loaded. However,
due to a bug in load_backend_if_not_loaded it was actually always
loading the environment for every call, even if the database env was
already loaded, leading to very expensive calls for nothing.

The premise of the method `with_dbenv` is that it will load the profile
and corresponding database environment if not already loaded. However,
due to a bug in `load_backend_if_not_loaded` it was actually *always*
loading the environment for every call, even if the database env was
already loaded, leading to very expensive calls for nothing.
@codecov
Copy link

codecov bot commented Apr 21, 2021

Codecov Report

Merging #4865 (6f2a8bc) into develop (c8ee9e6) will decrease coverage by 0.01%.
The diff coverage is 40.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4865      +/-   ##
===========================================
- Coverage    79.62%   79.62%   -0.00%     
===========================================
  Files          519      519              
  Lines        37109    37110       +1     
===========================================
- Hits         29546    29544       -2     
- Misses        7563     7566       +3     
Flag Coverage Δ
django 74.36% <40.00%> (-0.01%) ⬇️
sqlalchemy 73.27% <40.00%> (-<0.01%) ⬇️

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

Impacted Files Coverage Δ
aiida/cmdline/utils/decorators.py 78.58% <40.00%> (-5.06%) ⬇️

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 c8ee9e6...6f2a8bc. Read the comment docs.

@sphuber
Copy link
Contributor Author

sphuber commented Apr 26, 2021

@ramirezfranciscof also this would be good to quickly merge and release with 1.6.2

@ramirezfranciscof
Copy link
Member

ramirezfranciscof commented Apr 26, 2021

@sphuber is there a way to add a quick test that uses this method twice and somehow checks that there was not a double load? It would be nice to have, but I do recognize that it is not clear to me how it could check for that...

@sphuber
Copy link
Contributor Author

sphuber commented Apr 26, 2021

@sphuber is there a way to add a quick test that uses this method twice and somehow checks that there was not a double load? It would be nice to have, but I do recognize that it is not clear to me how it could check for that...

It could be done: we would have to mock the database loading env by a function that counts the number of times it is called with a global and raises the second time. But I didn't have the time to implement it and don't think it was necessary for this change. Doubt that this will accidentally be reverted at some point.

@ramirezfranciscof
Copy link
Member

Ok, I'll just open an issue then and we can just merge it as is (maybe if its quick and we don't have other problems I can even take care of it during the coding days after merging the new repository).

Copy link
Member

@ramirezfranciscof ramirezfranciscof left a comment

Choose a reason for hiding this comment

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

Good to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The aiida.cmdline.utils.decorators.with_dbenv loads database environment even if it is already loaded
2 participants