Change Fido.fetch() to raise an error if one or more files failed to download

[ayshih]

Currently, Fido.fetch() does not interrupt execution with an error if one or more files failed to download. This results in confusing behavior for non-interactive scripts, such as our gallery examples, where the script fails later down because the code then tries to work with non-existent filenames. I think that Fido.fetch() should raise an error, after all downloads have been attempted, so the script does not keep executing past the fetch() call. I suspect that in the majority of cases, the script will want all the files to have been successfully downloaded before continuing. In the minority of cases where the script would want to keep going even with failed downloads, the fetch() call can simply be wrapped in a try...except block.

I presume that raising an error means that you wouldn't get out parfive.Results? That admittedly would be annoying for interactive use... Maybe this error-raising behavior could be opt-in.

[alasdairwilson]

I think partial downloads should not block subsequent lines from running, maybe this just means examples should be better written

I think the only way I'd like this is with a strict kwarg or better a context manager or something.

[ryuusama09]

@ayshih , has any progress been made on this feature addition ? . If not , then I would like to work !

[Cadair]

@ryuusama09 I think the problem is we don't know what the best solution is.

My opinion is that raising an error in fetch() is a "bad idea" ™️ , a better thing to do would be to put in the gallery examples (and otherwise document the suggestion of doing:

res = Fido.fetch()
if res.errors:
    raise Exception("Downloads did not complete")

[alasdairwilson]

I totally agree that it's a bad idea to have fetch error: you essentially force users to use try blocks any time they have a fetch call. It's equivalent to an API change in the extent it changes behaviour

Your snippet is better but I would worry that manually checking the results of every fetch call means every example looking very unclean for no gain (over their current behaviour). If people want to do this in their own code they already can.

I'd personally prefer then that we make a users job (of interrupting if files do not download) easier.

files = fido.fetch(query, error_on_fail=True)

But even then I'm not convinced that this would be worth adding to our example gallery.

[nabobalis]

Do we want to only handle this for examples or in general?

[ryuusama09]

I totally agree that it's a bad idea to have fetch error: you essentially force users to use try blocks any time they have a fetch call. It's equivalent to an API change in the extent it changes behaviour

Your snippet is better but I would worry that manually checking the results of every fetch call means every example looking very unclean for no gain (over their current behaviour). If people want to do this in their own code they already can.

I'd personally prefer then that we make a users job (of interrupting if files do not download) easier.

files = fido.fetch(query, error_on_fail=True)

But even then I'm not convinced that this would be worth adding to our example gallery.

This snippet looks better . We can set an argument like you provided based on user's will (interrupt the call if error found , else continue ) . if results.error is not an empty object then we can throw an error. However this again might not be a suitable option if interrupting is not preferred .

[ayshih]

My opinion is that raising an error in fetch() is a "bad idea" ™️

I continue to be thoroughly baffled by this perspective. If fetch() has failed at its one job - downloading all files - why shouldn't that raise an error? What normal code following the fetch() call would still produce the intended output irrespective of download failures?

I totally agree that it's a bad idea to have fetch error: you essentially force users to use try blocks any time they have a fetch call.

If the user doesn't actually do anything with the downloaded files, then sure, those users would now need to do more. But, as our gallery examples show, users who want to do something with the downloaded files are currently forced to add code to check for download errors so that later code doesn't crash. Raising an error when the download failed makes more sense to me.

[ryuusama09]

My opinion is that raising an error in fetch() is a "bad idea" ™️

I continue to be thoroughly baffled by this perspective. If fetch() has failed at its one job - downloading all files - why shouldn't that raise an error? What normal code following the fetch() call would still produce the intended output irrespective of download failures?

I totally agree that it's a bad idea to have fetch error: you essentially force users to use try blocks any time they have a fetch call.

If the user doesn't actually do anything with the downloaded files, then sure, those users would now need to do more. But, as our gallery examples show, users who want to do something with the downloaded files are currently forced to add code to check for download errors so that later code doesn't crash. Raising an error when the download failed makes more sense to me.

Exactly like i said , we can leave the option of interrupt to the user by providing an extra arg ?!

[alasdairwilson]

Exactly like i said , we can leave the option of interrupt to the user by providing an extra arg ?!

@ryuusama09 I believe Albert is making the opposite point: that error should be default behaviour, not controlled by a kwarg.

I continue to be thoroughly baffled by this perspective. If fetch() has failed at its one job - downloading all files - why shouldn't that raise an error?

I think this is overly prescriptive on how people use sunpy; solarmonitor is my most obvious use case where we want to proceed at all costs there should be 7 files but we get 6, process the 6 that we got and get the 7th later.

It is totally normal as well, in a single script, to have multiple fetch() calls where they are not interdependent and execution would be halted by the first failure. It is also totally normal for peoples Fido queries to have more data than they need, they fancy seeing what aia saw of a flare, they might fire in an hour of data centered on the flare at 1 minute cadence, they might want 1000 data files to see what the average total DN is over a month, if 1 file fails to download they likely don't care but would still like onwards analysis to run.

add code to check for download errors so that later code doesn't crash.

I am not sure I see this as such a crime, you are suggesting we put an error earlier:

q =  Fido.query(...) # we expect 3 files
r = Fido.fetch(q) # we get 2 files
m = Map(r) # we Map 2 files
m[2].plot() # IndexError

I don't see this behaviour as that terrible.

Most importantly though, it is my opinion that throwing exceptions should be for truly exceptional things, it is normal for data providers to fail on us. It is also preferable, to me, for code to run as long is it is able to and not be prematurely stopped because it might fail later on.

[ryuusama09]

though

@ryuusama09 I believe Albert is making the opposite point: that error should be default behaviour, not controlled by a kwarg.

Oh , I guess i was contradicting . I think it is getting difficult to settle on an opinion 😅

[ayshih]

that error should be default behaviour, not controlled by a kwarg.

Yup, I'm arguing that the default behavior should be to raise an error, after all of the downloads have been attempted.

I continue to be thoroughly baffled by this perspective. If fetch() has failed at its one job - downloading all files - why shouldn't that raise an error?

I think this is overly prescriptive on how people use sunpy; solarmonitor is my most obvious use case where we want to proceed at all costs there should be 7 files but we get 6, process the 6 that we got and get the 7th later.

I'm arguing that raising an error is the best design choice, and that handling errors is normal. Users who know that their code is safe to proceed even with download failures can use try...except to handle the error. If try...except is too burdensome, we could also add a keyword (e.g., ignore_failures, with the default as False).

I'll claim that if your code inspects .errors to identify which files failed to download and why, then you're already handling errors. Having to handle errors through try...except should not be something that a Python coder should be allergic to.

I'll also point out that a user downloading files over a time range may end up being unclear whether the reason they do not have an exposure at a certain time is because the observation genuinely does not exist or because the download happened to fail.

Most importantly though, it is my opinion that throwing exceptions should be for truly exceptional things

Exceptions are very useful! I don't understand this resistance.

it is normal for data providers to fail on us.

This should not be "normal", and our code design should not enshrine this perspective.

It is also preferable, to me, for code to run as long is it is able to and not be prematurely stopped because it might fail later on.

This is bonkers to me.

Silly example 1: Converting a Quantity to an incompatible unit currently raises an error. You wouldn't argue that since we don't know that the user will actually use the converted output that we should fail silently (e.g., return output that is flagged as invalid) and force the user to then add protection code after conversions to make sure that the output is actually valid before continuing.

Silly example 2: Suppose I place an order from Amazon for 5 items, but it turns out that Amazon ran out of stock on 2 of them, so it ships me only 3 items. I would want Amazon to proactively alert me to the non-deliveries, rather than being silent on the off chance that I might be okay with not receiving those 2 items.

[nabobalis]

though

@ryuusama09 I believe Albert is making the opposite point: that error should be default behaviour, not controlled by a kwarg.

Oh , I guess i was contradicting . I think it is getting difficult to settle on an opinion 😅

I recommend looking at another issue while we try to work out what the best way forward is.

[ryuusama09]

understand thi

Sure @nabobalis

[alasdairwilson]

I'll claim that if your code inspects .errors to identify which files failed to download and why, then you're already handling errors. Having to handle errors through try...except should not be something that a Python coder should be allergic to.

But this wasn't in any of my examples, I gave concrete examples on use cases which are completely legitimate and would be completely impossible using a try block - though still possible if the exception can be bypassed with a kwarg.

The issue is that if an exception is raised in fetch then the user gets no data, whereas currently we get partial data. I don't like removing the ability to get partial data "just wrap it in a try block" does not make the function return the files. So because it is unclear, even though res.errors exists, why files have not been downloaded this whole functionality is for the chop.

I'll also point out that a user downloading files over a time range may end up being unclear whether the reason they do not have an exposure at a certain time is because the observation genuinely does not exist or because the download happened to fail.

I mean this is a good point, but both this and your amazon example points far more towards a warning than an exception, to make sure the user is aware that they might be missing data.

This should not be "normal", and our code design should not enshrine this perspective.

We should be working in a realistic way which is that it is not our fault that data is not downloaded but we can work around that rather than prevent the user from working around that.

We have no control over data providers and partial downloads are extremely common. Trying to download a month of SRS txt files and you will likely fail on 20%+ of them (so just run it a few times and you are normally fine).

Incidentally, presumably we are also deleting the successful files when an exception is raised because otherwise we are downloading files for the user but then not providing them paths to these files (or indeed any information at all on those files).

The lack of partial downloads means this can ONLY go in as an optional behaviour, if at all, I would strongly urge that to be opt in rather than opt out, first of all to prevent changing existing behaviour but also, despite your vehemence of the opposite, I do not see how no data is better than some data. In truth, I think this fits more as a warning:

"WARNING: some of your requested files were not downloaded, please check res.errors for more information"