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 optional modifier dependencies being unloaded for delayed composites #754

Merged
merged 4 commits into from
May 6, 2019

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented May 6, 2019

I noticed this while working with MERSI-2 data where some resolutions and modifiers didn't work out as desired and required composites to be delayed until after resampling. The basic idea is:

  1. Try loading a modified dataset where the modifier has optional dependencies and one of the required dependencies of the modifier or dataset causes a delayed generation (incompatible areas).
  2. If we detect that a hard requirement for a dataset or composite can not be loaded/created immediately then we mark the dependencies as "keepable" so they won't be removed. The requested dataset will be attempted again after resampling.
  3. We then stop checking for optional dependencies and do not try to generate the full composite (because we're missing some dependencies). We return to continue processing other requested datasets.

The issue is that in step 3 we aren't processing optional dependencies so they are never marked as "keepable". This results in optional dependencies being removed and not being available when the composite's generation is re-attempted after resampling.

  • Tests added
  • Tests passed
  • Passes git diff origin/master -- "*py" | flake8 --diff
  • Fully documented

@djhoese djhoese added bug component:dep_tree Dependency tree and dataset loading labels May 6, 2019
@djhoese djhoese requested a review from mraspaud May 6, 2019 17:06
@djhoese djhoese self-assigned this May 6, 2019
satpy/tests/utils.py Outdated Show resolved Hide resolved
satpy/tests/utils.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented May 6, 2019

Coverage Status

Coverage increased (+0.02%) to 80.708% when pulling e4fe32a on djhoese:bugfix-modifier-optionals into d0ff651 on pytroll:master.

@codecov
Copy link

codecov bot commented May 6, 2019

Codecov Report

Merging #754 into master will increase coverage by 0.01%.
The diff coverage is 96.96%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #754      +/-   ##
=========================================
+ Coverage   80.68%   80.7%   +0.01%     
=========================================
  Files         149     149              
  Lines       21650   21677      +27     
=========================================
+ Hits        17469   17495      +26     
- Misses       4181    4182       +1
Impacted Files Coverage Δ
satpy/tests/utils.py 96.96% <100%> (ø) ⬆️
satpy/tests/test_scene.py 99.43% <100%> (ø) ⬆️
satpy/scene.py 89.91% <93.75%> (+0.01%) ⬆️

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 d0ff651...e4fe32a. Read the comment docs.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple cosmetic changes

satpy/scene.py Outdated
@@ -765,7 +771,9 @@ def _get_prereq_datasets(self, comp_id, prereq_nodes, keepables, skip=False):
LOG.debug("Delaying generation of %s because of dependency's delayed generation: %s", comp_id, prereq_id)
if not skip:
LOG.debug("Missing prerequisite for '{}': '{}'".format(comp_id, prereq_id))
Copy link
Member

Choose a reason for hiding this comment

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

In this case, the prerequisite isn't missing, it is delayed. Could we modify the debug message to differentiate from where the prereq is actually missing ?

satpy/scene.py Outdated
@@ -788,12 +796,20 @@ def _generate_composite(self, comp_node, keepables):
compositor, prereqs, optional_prereqs = comp_node.data

try:
missing_prereq = False
Copy link
Member

Choose a reason for hiding this comment

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

Here again, I would use 'missing', maybe delayed_prereq ?

@mraspaud mraspaud merged commit 9f7dced into pytroll:master May 6, 2019
@djhoese djhoese deleted the bugfix-modifier-optionals branch May 7, 2019 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component:dep_tree Dependency tree and dataset loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants