Read of model when update in same root

[hboisgon]

Issue addressed

Fixes #256

Explanation

See #256 for discussions

Checklist

• Updated tests or added new tests
• Branch is up to date with main
• Tests & pre-commit hooks pass
• Updated documentation if needed
• Updated changelog.rst if needed

[savente93]

@savente93 pick this up

[hboisgon]

@savente93 I put some placeholders in the code where I think you can try to add filehandling. Thanks again in advance for the help!

[hboisgon]

Hi @savente93 , thanks for the help! I tested and seems to work like a charm for our test bench :)
The only thing I am not too sure about is what the forceful_cleanup option does? It seems it's set to False anyway in the tests and works.
If it's needed, please leave and then I added a CLI --fo option (similar to build) so that CLI user can also set it.

After that, could you do the final review? Unless @DirkEilander you prefer to also have a look?

[savente93]

To give a bit more context for the forced_cleanup flag: what the code does is just try to overwrite the destination file with the temporary one until either the maximum number of tries is reached or until it succeeds which is meant to solve timing issues (e.g. the file was closed but we still got a permission error because of data races). The forced cleanup also attempts to close the original file handle, which may or may not be necessary, but could cause trouble if the user will try to read from the same file handle after this function is called. it just seemed like a good idea to give the user a bit more flexibility, but if this is not necessary we could just remove it. Happy to defer to your judgement

[hboisgon]

Thanks! Then it's clear to me and I think a good idea indeed so let's keep it :)
It's ready for review then

[savente93]

few small comments, but other than that, I think she's ready to go chief!

[DirkEilander]

Nice work @hboisgon and @savente93 !
I've added a few questions / comments, but nothing major.

[DirkEilander]

Should we refer to internal function _write_nc in the docstring, which itself does not have a docstring? That's not a very helpful reference for users.

The _write_nc method could be added to the external api (remove underscore) with a docstring. It could potentially even be moved to the io.py script. Alternately, we could mention the underlying xarray.to_netcdf method here as those are the kwargs that users will mostly adapt no? idem in other places where this method is referenced.

[savente93]

Maybe it's a reflex, but if possible I'd prefer keeping things private as much as we can, so I would prefer refering to xarray.to_netcdf instead. @hboisgon what do you think?

[hboisgon]

I saw that we referenced it in a lot of places already but without an API docstring. The thing is that for the equivalent _read_nc method, the new 'load' option I implemented is actually then not passed to the open_dataset **kwargs so I have to document this better anyhow. I think these functions could easily be moved to io.py if you also agree?

[savente93]

I don't have strong feelings on moving it to io.py but if there is a way you think that the documentation could be improved, then I'm always happy with that. I'll leave it up to you though, I trust your judgement.

[hboisgon]

I did not realize but as @savente93 implemented the nice temporary file writting and saving to defered file list, it became quite tough to move this function to io.py. So in the end I decide to leave both read_nc and write_nc in model_api.py, removed the underscore and added docstrings. Should all be consistent now!