add kwarg to handle invalid files in open_mfdataset

[pratiman-91]

• Closes better handling of invalid files in open_mfdataset #6736
• User visible changes (including notable bug fixes) are documented in whats-new.rst

Added new argument in open_mfdataset to better handle the invalid files.

errors : {'ignore', 'raise', 'warn'}, default 'raise'
        - If 'raise', then invalid dataset will raise an exception.
        - If 'ignore', then invalid dataset will be ignored.
        - If 'warn', then a warning will be issued for each invalid dataset.

[welcome]

Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient.
If you have questions, some answers may be found in our contributing guidelines.

[max-sixty]

I'm not the expert, but this looks reasonable! Any other thoughts?

Assuming no one thinks it's a bad idea, we would need tests.

[headtr1ck]

I think it is a good idea.

But the way it is implemented here seems overly complicated and repetitive.
I would suggest to revert the logic: first build up the list wrapped in a single try and then handle the three cases in the except block.

[headtr1ck]

Almost there.

Also, we should add tests for this.

[pratiman-91]

@headtr1ck Thanks for the suggestions. I have added two tests (ignore and warn). Also, while testing, I found that a new argument broke combine="nested" due to invalid ids. I have now modified it to reflect the correct ids, and it is passing the tests. Please review the tests and the latest version.

[pratiman-91]

Hi @headtr1ck, I have been thinking about the handling of ids. Current version looks like a patch work (I am not happy with it.). I think we can create ids after removing all the invalid datasets from path1d within the combine==nested block. Please let me know what do you think.
Thanks!

[pratiman-91]

@max-sixty Can you please go through the PR. Thanks!

[max-sixty]

I'm admittedly much less familiar with this section of the code. nothing seems wrong though!

I think we should bias towards merging, so if no one has concerns then I'd vote to merge

could we fix the errors in the docs?

[pratiman-91]

It seems like one test failed test_sparse_dask_dataset_repr (xarray.tests.test_sparse.TestSparseDataArrayAndDataset) . It is not related to this PR.

[kmuehlbauer]

@headtr1ck Would you mind having one last look here? I'm not able to merge this without your interaction wrt to a requested change. Thanks!

[kmuehlbauer]

I'll close and reopen, just to check how this affects the merge rules.

[headtr1ck]

@headtr1ck Would you mind having one last look here? I'm not able to merge this without your interaction wrt to a requested change. Thanks!

Sorry for the late reply, currently on holidays.

I am not sure about this approach.

For 1D arrays it works fine (even though maybe users want to have the options to have NaNs in positions of missing values, but due to xarrays indexing this should not be an issue).

But for ND nested lists, basically this PR only works if the user supplies an additional file in the place of the broken file. This implies that the user actually knows that one file is broken. So I wonder if this is useful at all?

I think the main use case is the following:
[["1.nc", "2.nc"], ["broken.nc", "4.nc"]]
Now the question is what should actually happen here... I think the only way out is to fill in the gap with NaNs.

But maybe this is supposed to be an additional PR?

[kmuehlbauer]

Thanks @headtr1ck, I've had similar concerns here: #9955 (comment).

@pratiman-91's argument was, if the provided nested list would lead to a consistent output (after removal of any breaking file) it should return without erroring out. It's hard to imagine a real life use case, when and how this could happen. @pratiman-91 can you please elaborate your use case here?

I think the main use case is the following: [["1.nc", "2.nc"], ["broken.nc", "4.nc"]] Now the question is what should actually happen here... I think the only way out is to fill in the gap with NaNs.

But maybe this is supposed to be an additional PR?

I'd totally support this approach, although this seems not an easy task. But should go in a follow-up PR.

From my side, this is PR is still a first way forward, giving the user a bit more freedom when opening/combining nested lists of files.

[pratiman-91]

Thanks, @headtr1ck and @kmuehlbauer.

I think the main use case is the following:
[["1.nc", "2.nc"], ["broken.nc", "4.nc"]]
Now the question is what should actually happen here... I think the only way out is to fill in the gap with NaNs.

In this case, two things will happen:

1. If removing broken.nc results in a logically valid dataset, then we proceed with the rest.

2. If it still doesn't form a valid dataset, an error should be raised.

Filling with NaNs introduces its own set of challenges. For instance, if the first file itself is broken, there's no reliable source for metadata needed to correctly generate NaNs. This approach becomes significantly more complex than simply skipping the broken file and would require a separate PR to handle properly.

[headtr1ck]

Some minor changes are still required.

Another question: what happens now if someone passes a e.g. 2x2 list of files where one is broken?

Because as far as I can tell, if errors="ignore" this file will be silently removed but then later on the dataset cannot be constructed and quite likely will throw an error that will confuse the user.

[pratiman-91]

@headtr1ck

Some minor changes are still required.

I have made changes based on your suggestions.

Another question: what happens now if someone passes a e.g. 2x2 list of files where one is broken?

Because as far as I can tell, if errors="ignore" this file will be silently removed but then later on the dataset cannot be constructed and quite likely will throw an error that will confuse the user.

I agree, that would be the case. An important assumption is that removing the files does not affect the overall validity of the datasets. I think it should be up to the user to use that option.