Fix MultiScene writer handling of multiple delayed objects #1639

BENR0 · 2021-04-13T10:28:25Z

To support groups the cf_writer was implemented in a way that even if no groups were specified two delayeds were returned. This made it impossible to use distributed writing with multiscenes.
Now if now groups are used a single delayed is returned since all datasets can be written at once without appending to the file.

Tests added

If no groups are specified the cf_writer now returns only one delayed object. Therefore distributed writing of netcdfs is now possible with Multiscenes.

djhoese

I don't know much about the internals of the CF writer, but what about having multiple delayed objects returned stops MultiScene from working?

djhoese · 2021-04-13T11:59:40Z

satpy/multiscene.py

-                    log.info("Finished saving %d scenes", idx)
-                    idx += 1
+                # for future in future_list:
+                future_list.result()


Was this left over from testing or is the for loop actually not needed?

BENR0 · 2021-04-13T13:02:56Z

@djhoese scratch that. Sorry for the confusion. You are quite right in general there is no problem with multiple delayed objects hence the for loop. I experimented with this a while ago and I fixed something at the wrong point (fixed the cf writer to return only one delayed which in turn needed removal of the loop). What I think actually needs fixing is:

satpy/satpy/multiscene.py

Line 348 in 2245c9a

if isinstance(delayed, (list, tuple)) and len(delayed) == 2:

While this catches situations with writers returning (source, target) combinations it also catches the cf writer case because it always returns a list of at least two delayed objects. This if clause should be more specific so it does not apply to the cf writer.

djhoese · 2021-04-13T13:08:06Z

Ah yes, that does seem like too simplistic of a condition. There may be utilities in satpy/writers/__init__.py that could be used to "parse" the return values from the writers.

BENR0 · 2021-04-19T12:16:39Z

I checked satpy/writers/__init__.py for something useful but could not find something directly applicable. But if I see it correctly if a reader has sources they are always the first item so adding and isintance(delayed[0], da.Array) to the condition would solve the issue I think.

The changes to the reader are strictly not necessary then. The only "advantage" being that in the case with no groups only one delayed will be returned and therefore only one file access is needed but I am not sure if that has any relevant benefit. So I would propose to add the additional condition in the distributed writing and roll back the changes in the writer and maybe keep some of the code simplifying changes only.

djhoese · 2021-04-19T15:28:19Z

The MultiScene could use split_results:

satpy/satpy/writers/__init__.py

Lines 487 to 514 in 3984453

    
           def split_results(results): 
        
               """Split results. 
        
               Get sources, targets and delayed objects to separate lists from a list of 
        
               results collected from (multiple) writer(s). 
        
               """ 
        
               from dask.delayed import Delayed 
        
               def flatten(results): 
        
                   out = [] 
        
                   if isinstance(results, (list, tuple)): 
        
                       for itm in results: 
        
                           out.extend(flatten(itm)) 
        
                       return out 
        
                   return [results] 
        
               sources = [] 
        
               targets = [] 
        
               delayeds = [] 
        
               for res in flatten(results): 
        
                   if isinstance(res, da.Array): 
        
                       sources.append(res) 
        
                   elif isinstance(res, Delayed): 
        
                       delayeds.append(res) 
        
                   else: 
        
                       targets.append(res) 
        
               return sources, targets, delayeds

and if there are any sources then raise the error. Although, if this function is used here it should maybe be renamed to something like sort_writer_results.

codecov · 2021-04-19T15:42:41Z

Codecov Report

Merging #1639 (3a07044) into main (2a1110e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1639   +/-   ##
=======================================
  Coverage   92.66%   92.67%           
=======================================
  Files         258      258           
  Lines       37988    38024   +36     
=======================================
+ Hits        35200    35237   +37     
+ Misses       2788     2787    -1

Flag	Coverage Δ
behaviourtests	`4.82% <2.27%> (-0.01%)`	⬇️
unittests	`92.80% <100.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
satpy/multiscene.py	`91.10% <100.00%> (+0.33%)`	⬆️
satpy/tests/test_multiscene.py	`99.46% <100.00%> (+0.04%)`	⬆️
satpy/writers/cf_writer.py	`95.17% <100.00%> (-0.02%)`	⬇️
satpy/writers/geotiff.py	`90.90% <0.00%> (+0.16%)`	⬆️
satpy/tests/writer_tests/test_geotiff.py	`97.87% <0.00%> (+0.19%)`	⬆️

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 2a1110e...3a07044. Read the comment docs.

BENR0 · 2021-04-20T11:39:15Z

Yes that's were I got the idea from. I am not really that familiar with all the writers and how their return values look like. It is very possible that what I proposed is not covering it and split_writer_results should be used. I was just thinking that it over complicates things if the small addition to the condition will fix the issue.

If using split_writer_results is preferred I will change it and push it again.

djhoese · 2021-04-20T12:15:20Z

I don't think there is anything logically wrong with what you're suggesting, but if you use split_writer_results then we will catch issues like this sooner if writer results are changed in the future. And when developers need to change split_writer_results they will see it is being used in multiscene.py and that it also needs to be updated.

BENR0 · 2021-05-11T13:43:12Z

Sorry for the delay. I changed it to use split_results now.

djhoese · 2021-05-11T13:45:19Z

satpy/multiscene.py

@@ -345,7 +345,8 @@ def load_data(q):

        for scene in scenes_iter:
            delayed = scene.save_datasets(compute=False, **kwargs)
-            if isinstance(delayed, (list, tuple)) and len(delayed) == 2:
+            sources, targets, _ = split_results(delayed)


Shouldn't the _ be overwriting delayed? This way, in the future, if the MultiScene can handle source/target combinations and delayed objects the code won't have to be changed. Right?

~~Good point.~~ I thought about it and I think it should not be overwritten since the result of split_results is only needed for the check of sources. Currently delayed is allowed to be a target/ list of targets (e.g. simple image writer) or a delayed/ list of delayeds (e.g. CF writer). So if delayed got overwritten some additional code to either hand targets or delayed to client.compute in line 355 would be needed. So for the check only sources is needed and targets could also be _.

One thing I noticed was that it is not well documented what each writer returns.

Targets should never be returned without a source. They are source + target pairs. The simple image writer should return delayed objects. The geotiff writer returns source (dask array) + target (output file-like object). If you have another example, then I still propose that _ be replaced with delayed.

Ok I see now I got fooled by the test which returned targets without sources from split_results for the simple image writer because the mock did not pass as a delayed object. I changed the code to overwrite delayed now and also fixed the test. Additionally I added a test for writers with source/target combinations to fail.

djhoese · 2021-05-13T13:30:11Z

I re-titled this after it seemed like most of the work was in getting the MultiScene to work. However, I realize you still did a lot of work on the CF writer. Is this work still needed? If so, would you mind merging with the pytroll main branch and resolve the conflicts in the CF writer?

djhoese · 2021-05-15T16:37:19Z

satpy/multiscene.py

@@ -341,17 +341,20 @@ def load_data(q):

        input_q = Queue(batch_size if batch_size is not None else 1)
        load_thread = Thread(target=load_data, args=(input_q,))
+        # set threads to daemon so they are killed if error is raised from main thread
+        load_thread.daemon = True


This can be set in the Thread init right?

Yes indeed. Changed it.

satpy/tests/test_multiscene.py

djhoese

LGTM thanks!

BENR0 added 2 commits February 25, 2021 08:12

fix: cf_writer returning two delayeds

731851f

If no groups are specified the cf_writer now returns only one delayed object. Therefore distributed writing of netcdfs is now possible with Multiscenes.

refactor: duplicate code for group/non group netcdf

55a2990

BENR0 requested review from djhoese and mraspaud as code owners April 13, 2021 10:28

djhoese reviewed Apr 13, 2021

View reviewed changes

refactor: use split_results in distributed save

2ea8e03

djhoese reviewed May 11, 2021

View reviewed changes

BENR0 added 2 commits May 13, 2021 11:01

refactor: overwrite delayd with result from split_results

7b8eb09

refactor: distributed save test

7467258

djhoese changed the title ~~Fix cf writer returning two delayeds~~ Fix MultiScene writer handling of multiple delayed objects May 13, 2021

djhoese added bug component:multiscene labels May 13, 2021

djhoese self-assigned this May 13, 2021

BENR0 added 2 commits May 14, 2021 13:52

refactor: remove return of only one delayed for none group netcdf

440b9b3

Merge branch 'upstream_main' into fix_cf_writer_returning_two_delayeds

3438a5b

djhoese reviewed May 15, 2021

View reviewed changes

satpy/tests/test_multiscene.py Show resolved Hide resolved

refactor: move imports

3a07044

djhoese approved these changes May 17, 2021

View reviewed changes

djhoese merged commit 62c0e83 into pytroll:main May 17, 2021

BENR0 deleted the fix_cf_writer_returning_two_delayeds branch December 4, 2023 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix MultiScene writer handling of multiple delayed objects #1639

Fix MultiScene writer handling of multiple delayed objects #1639

BENR0 commented Apr 13, 2021

djhoese left a comment

djhoese Apr 13, 2021

BENR0 commented Apr 13, 2021

djhoese commented Apr 13, 2021

BENR0 commented Apr 19, 2021

djhoese commented Apr 19, 2021

codecov bot commented Apr 19, 2021 •

edited

Loading

BENR0 commented Apr 20, 2021

djhoese commented Apr 20, 2021

BENR0 commented May 11, 2021

djhoese May 11, 2021

BENR0 May 12, 2021 •

edited

Loading

djhoese May 12, 2021

BENR0 May 13, 2021

djhoese commented May 13, 2021

djhoese May 15, 2021

BENR0 May 17, 2021

djhoese left a comment

Fix MultiScene writer handling of multiple delayed objects #1639

Fix MultiScene writer handling of multiple delayed objects #1639

Conversation

BENR0 commented Apr 13, 2021

djhoese left a comment

Choose a reason for hiding this comment

djhoese Apr 13, 2021

Choose a reason for hiding this comment

BENR0 commented Apr 13, 2021

djhoese commented Apr 13, 2021

BENR0 commented Apr 19, 2021

djhoese commented Apr 19, 2021

codecov bot commented Apr 19, 2021 • edited Loading

Codecov Report

BENR0 commented Apr 20, 2021

djhoese commented Apr 20, 2021

BENR0 commented May 11, 2021

djhoese May 11, 2021

Choose a reason for hiding this comment

BENR0 May 12, 2021 • edited Loading

Choose a reason for hiding this comment

djhoese May 12, 2021

Choose a reason for hiding this comment

BENR0 May 13, 2021

Choose a reason for hiding this comment

djhoese commented May 13, 2021

djhoese May 15, 2021

Choose a reason for hiding this comment

BENR0 May 17, 2021

Choose a reason for hiding this comment

djhoese left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 19, 2021 •

edited

Loading

BENR0 May 12, 2021 •

edited

Loading