Skip to content

Conversation

@jklymak
Copy link
Contributor

@jklymak jklymak commented Apr 7, 2025

Without this, xarray emits a bunch of warnings.

@mjlosch
Copy link
Member

mjlosch commented Aug 5, 2025

on the master branch I get for test_mds_store.py and python 3.11 (also 3.13, didn't check 3.12):

  /Users/mlosch/MITgcm/xmitgcm/xmitgcm/mds_store.py:303: FutureWarning: In a future version, xarray will not decode timedelta values based on the presence of a timedelta-like units attribute by default. Instead it will rely on the presence of a timedelta64 dtype attribute, which is now xarray's default way of encoding timedelta64 values. To continue decoding timedeltas based on the presence of a timedelta-like units attribute, users will need to explicitly opt-in by passing True or CFTimedeltaCoder(decode_via_units=True) to decode_timedelta. To silence this warning, set decode_timedelta to True, False, or a 'CFTimedeltaCoder' instance.
    ds['time'] = xr.decode_cf(ds[['time']])['time']

I guess that's what this PR addresses. I also get this:

  /Users/mlosch/MITgcm/xmitgcm/xmitgcm/mds_store.py:303: SerializationWarning: Unable to decode time axis into full numpy.datetime64[ns] objects, continuing using cftime.datetime objects instead, reason: dates out of range. To silence this warning use a coarser resolution 'time_unit' or specify 'use_cftime=True'.
    ds['time'] = xr.decode_cf(ds[['time']])['time']

with this branch the second warning remains:

  /Users/mlosch/MITgcm/xmitgcm/xmitgcm/mds_store.py:303: SerializationWarning: Unable to decode time axis into full numpy.datetime64[ns] objects, continuing using cftime.datetime objects instead, reason: dates out of range. To silence this warning use a coarser resolution 'time_unit' or specify 'use_cftime=True'.
    ds['time'] = xr.decode_cf(ds[['time']], decode_timedelta=True)['time']

@jklymak Is this something that this PR can also address?

@mjlosch
Copy link
Member

mjlosch commented Aug 5, 2025

... and since this PR reduces the number of warnings, we could try to address this warning:

  /Users/mlosch/MITgcm/xmitgcm/xmitgcm/test/test_xmitgcm_common.py:252: DeprecationWarning: Python 3.14 will, by default, filter extracted tar archives and reject files or modify their metadata. Use the filter argument to control this behavior.
    tar.extractall(target_dir)

by adding filter='data' or filter=None to the arguments of tar.extractall(target_dir,...). It also seems to work for python 3.11, although the filter-argument has only been introduced in 3.12 according to the tarfile-documentation. I don't see any side effects in the tests.

@jklymak
Copy link
Contributor Author

jklymak commented Aug 5, 2025

@mjlosch - for sure we can add to this PR to address other issues. If you have solutions, always feel free to push to my PRs or supersede them with your own if that is cleaner. I can probably look at this in a day or two. Thanks!

@mjlosch
Copy link
Member

mjlosch commented Aug 6, 2025

@jklymak As you can see from the recently merged PRs I seem to have merge-rights also for this repository. I am very interested in keeping xmitgcm up to date and working, but I am afraid that my python skills are not up to speed with many things in this repository. So I am relying entirely on other people's python skills and the recently fixed CI (thanks again to @IvanaEscobar for all the work she put into this). That means I will not really be able to proactively do anything without anyone looking over my shoulder. But I can merge PRs that pass the tests. So I'll wait for you to have a look at my suggestions and then you can decide, if you want to do anything about this or not and let me know. As it is this PR already removes most of the warnings. Thanks for your contribution!

@mjlosch
Copy link
Member

mjlosch commented Aug 6, 2025

On second thought, the

 /Users/mlosch/MITgcm/xmitgcm/xmitgcm/mds_store.py:303: SerializationWarning: Unable to decode time axis into full numpy.datetime64[ns] objects, continuing using cftime.datetime objects instead, reason: dates out of range. To silence this warning use a coarser resolution 'time_unit' or specify 'use_cftime=True'.
    ds['time'] = xr.decode_cf(ds[['time']], decode_timedelta=True)['time']

is probably triggered on purpose by using a non-standard calendar with a fictional start date far in the future in the corresponding test. So never mind, let's leave as it is.

I read a little about the tarfile.extractall warning here. To me it looks like we would be fine with just ignoring the warning, but to have fewer warnings, we can still add filter='data' now.
The new filter keyword was introduced in python 3.12 and defaults to 'fully_trusted' (the most liberal and unsafe method to extract a tar ball). In 3.14, the default will change to 'data' which is the most restrictive method. Specifying explicitly any filter value works and has no effect on the test, so we could just add any of filter='fully_trusted'/'tar'/'data'. Since we are unpacking our own files, there shouldn't be any security issues, I guess.

@mjlosch
Copy link
Member

mjlosch commented Aug 7, 2025

I added my tarfile.extractall modification to PR #354

so from my point of view, this PR is ready to be merged, unless there are any other objections.

@mjlosch mjlosch merged commit 472252b into MITgcm:master Aug 8, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants