arviz-devs / arviz

Exploratory analysis of Bayesian models with Python
https://python.arviz.org
Apache License 2.0
1.56k stars 386 forks source link

DeprecationWarning from `make_attrs` in Python 3.12 #2327

Closed ricardoV94 closed 3 months ago

ricardoV94 commented 3 months ago

We're running the pymc CI in Python 3.12 (https://github.com/pymc-devs/pymc/pull/7203) and seeing this warning: https://github.com/pymc-devs/pymc/actions/runs/8360787111/job/22887274333?pr=7203#step:7:4158

     def make_attrs(attrs=None, library=None):
        """Make standard attributes to attach to xarray datasets.

        Parameters
        ----------
        attrs : dict (optional)
            Additional attributes to add or overwrite

        Returns
        -------
        dict
            attrs
        """
        default_attrs = {
>           "created_at": datetime.datetime.utcnow().isoformat(),
            "arviz_version": __version__,
        }
E       DeprecationWarning: datetime.datetime.utcnow() is deprecated and scheduled for removal in a future version. Use timezone-aware objects to represent datetimes in UTC: datetime.datetime.now(datetime.UTC).
imperorrp commented 3 months ago

I dug into this a bit and tried the suggested alternative in the DeprecationWarning (current_time_new = datetime.datetime.now(datetime.UTC).isoformat()) using Python 3.9

However, doing so produces this error: AttributeError: module 'datetime' has no attribute 'UTC'

It seems datetime.UTC was only introduced in Python 3.11 but as an alias for datetime.timezone.UTC. So it might be good to update this code as this:

default_attrs = {
        "created_at": datetime.datetime.now(datetime.timezone.utc).isoformat(),
        "arviz_version": __version__,
    }

This could get rid of the deprecated code plus ensure it works with lower than the latest Python versions

ahartikainen commented 3 months ago

Did we still target python 3.9?

Cc @OriolAbril ?

ricardoV94 commented 3 months ago

There's still 3.10 right?

imperorrp commented 3 months ago

Did we still target python 3.9?

Cc @OriolAbril ?

There's still 3.10 right?

Oh yes, sorry, I just saw the main README says ArviZ is tested on Python 3.10 onwards. I shouldn't have used Python 3.9 to test it. So this edit could still be useful for now.

If support switches to 3.11 in the future then datetime.datetime.now(datetime.UTC) could be used.

OriolAbril commented 3 months ago

Thanks for checking @imperorrp, that is extremely useful. We have indeed dropped support for python 3.9 in the development version already, and next release will be 3.10 only. But we should still support 3.10 at least until roughly end of 2024, so we should go with your proposal instead of the code recommended by the error message.

If anyone can make a PR too it will be very welcome.

imperorrp commented 3 months ago

Sure, I can make a PR for this @OriolAbril

On it

imperorrp commented 3 months ago

One more thing- I did a search for any other similar datetime uses in the codebase just to make sure. In the concat function in arviz/data/inference_data.py,

current_time = str(datetime.now()) is used, which returns time in local timezone and not UTC. current_time then seems to be made the new "created_at" attribute with the previous such attribute/s put into a "previous_created_at" attribute.

I think this could be updated as well, to current_time = datetime.datetime.now(datetime.timezone.utc).isoformat() to maintain same style (isoformatting) plus using the UTC timezone.

ahartikainen commented 3 months ago

One more thing- I did a search for any other similar datetime uses in the codebase just to make sure. In the concat function in arviz/data/inference_data.py,

current_time = str(datetime.now()) is used, which returns time in local timezone and not UTC. current_time then seems to be made the new "created_at" attribute with the previous such attribute/s put into a "previous_created_at" attribute.

I think this could be updated as well, to current_time = datetime.datetime.now(datetime.timezone.utc).isoformat() to maintain same style (isoformatting) plus using the UTC timezone.

This sounds like a good idea