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

Disable caching for inline calculations #1872

Merged

Conversation

greschd
Copy link
Member

@greschd greschd commented Aug 13, 2018

Fixes #1871

Caching on inline calculations was broken since the switch to using the workfunction for their implementation, although unfortunately in a way that was not caught by the tests (see #1871).

Since caching isn't implemented for the workfunction, there is not immediate way to fix this. It's probably possible, but will work correctly only if the user adheres to the design rules for inline calcs, for example to not rely on some external parameter in the calculation.

Given all this, and the fact that inline calcs are supposed to be very fast operations anyway, the best course of action right now seems to me to just disable caching for the InlineCalculation.

@greschd greschd requested a review from sphuber August 13, 2018 15:35
@greschd greschd force-pushed the disable_caching_for_inlinecalcs branch from 84b0cc1 to 1ee37b9 Compare August 13, 2018 15:37
@giovannipizzi
Copy link
Member

Makes sense to me - @sphuber do you agree?

@codecov-io
Copy link

Codecov Report

Merging #1872 into develop will increase coverage by 5.31%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #1872      +/-   ##
==========================================
+ Coverage    61.38%   66.7%   +5.31%     
==========================================
  Files          320     320              
  Lines        32549   33420     +871     
==========================================
+ Hits         19981   22293    +2312     
+ Misses       12568   11127    -1441
Impacted Files Coverage Δ
...a/orm/implementation/general/calculation/inline.py 62.5% <100%> (ø) ⬆️
aiida/cmdline/utils/shell.py 16.94% <0%> (-83.06%) ⬇️
aiida/cmdline/utils/query/mapping.py 74.39% <0%> (-4.72%) ⬇️
...implementation/general/calculation/job/__init__.py 40.32% <0%> (-3.38%) ⬇️
aiida/common/caching.py 91.86% <0%> (-2.33%) ⬇️
aiida/daemon/execmanager.py 7.85% <0%> (-0.75%) ⬇️
aiida/cmdline/params/options/__init__.py 100% <0%> (ø) ⬆️
aiida/common/setup.py 74.84% <0%> (+0.2%) ⬆️
aiida/orm/implementation/general/node.py 78.5% <0%> (+0.31%) ⬆️
aiida/work/processes.py 95.67% <0%> (+0.32%) ⬆️
... and 39 more

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 dddfffc...1ee37b9. Read the comment docs.

@greschd
Copy link
Member Author

greschd commented Aug 27, 2018

We had some more discussion at #1871, but I'm not sure what the conclusion is.

@sphuber
Copy link
Contributor

sphuber commented Aug 27, 2018

Just that I wasn't sure of the exact reason for it breaking and I wanted to investigate it better but sadly I forgot a bit about it. I would be ok with merging this, but we should maybe open another issue to investigate the caching of function calculations.

@sphuber sphuber changed the base branch from develop to release_v0.12.3 December 17, 2018 14:36
@sphuber sphuber force-pushed the disable_caching_for_inlinecalcs branch from 1ee37b9 to c7ad8ae Compare December 17, 2018 16:08
@sphuber sphuber force-pushed the disable_caching_for_inlinecalcs branch from c7ad8ae to d0166c2 Compare December 17, 2018 16:29
@coveralls
Copy link

Coverage Status

Coverage increased (+7.9%) to 55.788% when pulling d0166c2 on greschd:disable_caching_for_inlinecalcs into a86dfed on aiidateam:release_v0.12.3.

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Approving this, since the actual problem is fixed in develop which will become v1.0.0 once released

@sphuber sphuber merged commit 3e96d16 into aiidateam:release_v0.12.3 Dec 17, 2018
@sphuber sphuber deleted the disable_caching_for_inlinecalcs branch December 17, 2018 16:40
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.

5 participants