Address ruff's DTZ003 rule violations

[hfcpeixoto]

Description

This pull request is to address ruff's DTZ003: "Checks for usage of datetime.datetime.utcnow()".

Replace utcnow() usages by now(tz=timezone.utc).

[github-actions]

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

[github-actions]

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

[pllim]

Thanks! I tried this as part of #14784 but I ran into timedelta math error. Perhaps you know how to fix? Please see #14784 (comment)

[hfcpeixoto]

Yeah, I ran into it as well. I proposed a solution, but I will leave it up for debate when the PR is ready for review. I will write some notes on this, I hope. Thanks!

[pllim]

I think users need to be made aware of the necessity to do this extra step, so it needs a change log?

[WilliamJamieson]

Possibly, however I am not sure if this should be the change. A better change here might be for Time.datetime to return a "timezone-aware" datetime by default? @mhvk what do you think should happen?

Note that I would like to see these changes merged as they should fix some of the issues we are seeing in Python 3.12 in #14784.

[nstarman]

See my comment above about now.

[mhvk]

I think for now we should simply also adjust the output for TimeDatetime to be UTC by default if the scale is UTC (keeping the present behaviour of not setting the timezone for any other scale). This would make changes at

astropy/astropy/time/formats.py

Lines 1126 to 1130 in bde1b85

	if timezone is not None:
	if self._scale != "utc":
	raise ScaleValueError(
	f"scale is {self._scale}, must be 'utc' when timezone is supplied."
	)

Essentially, one would change those lines to,

        if self._scale == "utc":
            if timezone is None:
                timezone = timezone.utc
        else:
            if timezone is not None:
                raise ScaleValueError(
                    f"scale is {self._scale}, must be 'utc' when timezone is supplied."
                )

With that, your change here would no longer be required.

[WilliamJamieson]

@mhvk, I think there are more subtleties to consider. This change would break the following:

from astropy.time import Time
import datetime

t = datetime.datetime.now()  # not timezone aware
a_t = Time(t)  # should be the same time

a_t.datetime - t  # raises the error about incompatible timezones with the proposed change, works currently

[hfcpeixoto]

Folks, I believe I am missing something, but this PR is only avoiding using calls to naive utcnow. Astropy's Time remains naive as before, and no API change is introduced.
Time::now() is still using an equivalent call to utcnow and return the same naive time.

I understand of course that fetching aware times and storing them as naive is throwing information away, but shouldn't this be addressed in a dedicated PR, which will most probably change Time's API?

I would even be willing to put some effort on it (with some guidance from someone more experienced in astropy).

Do you folks believe there is any improvement to be done in this PR? I would not mind working on additions, but it is not clear to me what should be done from this point on, given the discussions.

[pllim]

@hfcpeixoto , sorry for the confusion! Just replacing utcnow broke one of the tests, so that is why the changes blew up. Now it is up to the maintainers on what is the best path forward...

[mhvk]

@WilliamJamieson - I think my change would actually correct a bug, since datetime.now() would get the local time, while Time.now().datetime gives UTC. So, the two should generally not be equal. I think I slightly prefer to fix this to the best of our abilities.

Maybe let me ping @taldcroft too. Overall, it seems options are:

1. current PR, which means updating one test (if in the test we did now.replace(tzinfo=None), it would remain clear that Time itself did not change at all).
2. My update, so that Time.datetime always gives a timezone if scale="utc";
3. Filter warnings in TST: Test against Python 3.12 pre-prelease #14784.

[WilliamJamieson]

@mhvk my example has nothing to do with Time.now(), instead it considers the behavior one might expect from constructing a Time object (a_t) from a timezone unaware datetime object (t). The change you propose makes it so that Time.datetime always (in the context of construction from Python datetimeobjects) is timezone aware which is a major change from the current behavior. This change will break any existing code which computes a_t.datetime - t where the (Python datetime) t object is not timezone aware. I think it is perfectly reasonable for users to assume that their Time objects are not timezone aware unless they make them so, like what is done by the Python datetime.

The replacement of now = datetime.datetime.utcnow() with now = datetime.datetime.now(tz=timezone.utc) in the test is what is the actual behavior change at issue, not the change in the astropy code. This is because the change causes the now object to be timezone aware, which causes Python to then require the other time used when computing a timedelta with it to also be timezone aware. The fix for that issue is what lead @hfcpeixoto to using .replace to resolve. Instead the fix to preserve what is currently being tested should be for the timezone awareness to be removed from now not d_t.

This leaves the question of how Time objects can preserve timezone information which maybe encoded in a datetime object used to construct them. Currently, there is no mechanism to do so.

[WilliamJamieson]

@hfcpeixoto, sorry for creating the confusion for you. It just so happens that this PR makes most of the changes necessary to fix some of the problems identified in #14784, so it has taken on more importance than you originally intended. Thank you for attempting to resolve these seemingly small issues, it shows the utility in using more of the ruff rules. However, as in many cases, fixing seemingly small things reveals a larger issues.

[nstarman]

Thanks for tackling this challenging problem. Python has sub-optimal time zone support.

[nstarman]

I think a new (optional) argument should be added to now that allows the user to specify the timezone.
The default value should be gotten from time.tzname, corrected for daylight savings with 'time.localtime().tm_isdst (see https://stackoverflow.com/questions/1111056/get-time-zone-information-of-the-system-in-python and https://bugs.python.org/issue22798).

[nstarman]

I think time.localtime().tm_gmtoff will be necessary to get the offset from utc

[nstarman]

daylight savings corrections might use tzinfo.dst instead of tm_isdst. I haven't investigated details

[mhvk]

I like the idea of an optional argument, but it would be a big API change to make Time.now() return non-UTC times, so I think the default has to be timezone.utc. Obviously, this could also be done as follow-up, since here the idea is just to avoid the utcnow() method. Indeed, given the need to get this in for #14784, perhaps it is best to make minimal changes here.

[nstarman]

See my comment above about now.

[mhvk]

Good idea, but indeed somewhat tricky. Suggestions in-line.

[mhvk]

I like the idea of an optional argument, but it would be a big API change to make Time.now() return non-UTC times, so I think the default has to be timezone.utc. Obviously, this could also be done as follow-up, since here the idea is just to avoid the utcnow() method. Indeed, given the need to get this in for #14784, perhaps it is best to make minimal changes here.

[mhvk]

I think for now we should simply also adjust the output for TimeDatetime to be UTC by default if the scale is UTC (keeping the present behaviour of not setting the timezone for any other scale). This would make changes at

astropy/astropy/time/formats.py

Lines 1126 to 1130 in bde1b85

	if timezone is not None:
	if self._scale != "utc":
	raise ScaleValueError(
	f"scale is {self._scale}, must be 'utc' when timezone is supplied."
	)

Essentially, one would change those lines to,

        if self._scale == "utc":
            if timezone is None:
                timezone = timezone.utc
        else:
            if timezone is not None:
                raise ScaleValueError(
                    f"scale is {self._scale}, must be 'utc' when timezone is supplied."
                )

With that, your change here would no longer be required.

[WilliamJamieson]

Suggested change

	now = datetime.datetime.now(tz=datetime.timezone.utc)
	# `Time.datetime` is not timezone aware, meaning `.replace` is necessary for
	# `now` also not be timezone aware.
	now = datetime.datetime.now(tz=datetime.timezone.utc).replace(tzinfo=None)

This is my suggestion from https://github.com/astropy/astropy/pull/15298/files#r1323005153 along side changing dt back to:

dt = t.datetime - now

While leaving astropy.time.formats alone. This has two advantages:

1. It results in no behavioral changes from astropy.time.Time with respect to what Time.datetime is (not timezone aware).
2. It more accurately conveys what was actually being tested prior to these changes. That is computing "now" as UTC times, which are not timezone aware.

[mhvk]

Yes, good suggestion, helps to reproduce what is there and keep the PR minimal. The rest can be in follow-up!

[mhvk]

I opened #15311 to discuss possible future changes in how we deal with timezones.

[mhvk]

I didn't notice #15306 when I opened my issue for discussion; that would be the right place to continue discussion, so I closed my feature request. For this PR, I think the path forward is to follow @WilliamJamieson's #15298 (comment)

[hfcpeixoto]

Thanks, @mhvk. I have added a commit based on o @WilliamJamieson's suggestion #15298 (comment). Maybe I should re-request review at this point?

[mhvk]

With the latest change, this looks all good to me! Thanks, @hfcpeixoto!

[mhvk]

p.s. I added "refactoring" and "no-changelog-entry-needed" since those seemed appropriate.

[WilliamJamieson]

@hfcpeixoto I went ahead and rebased your PR and modified your last commit message to remove the @ sign. While I appreciate your attempt at crediting me, unfortunately the way GitHub notifications work in my experience is that I get one for any mention of my GitHub name which includes the @ sign in front. This includes when others rebase/push their branches off astropy main if/when this gets merged. This tends to blow up my notifications for a few days when it happens on something relatively popular like astropy, which is not desirable. Its not a huge problem as I was notified about this via the aforementioned notification system meaning I was able to go ahead and fix it for myself.

Again thank you for your timely contribution, which ended up sparking a deeper conversation than was expected. It is always nice to identify and eliminate technical debt.

[pllim]

Huh, weird, back in my Python 3.12 PR, I had to patch docs/time/index.rst but I don't see that here. I guess ruff does not care about doctest?

[WilliamJamieson]

Ruff only operates on pure Python files. It totally ignores any non Python files like the documentation rst files.

[pllim]

Then I guess it is a little dangerous to rely solely on it to check such things? For the future, please also do a recursive grep. Thanks.

[pllim]

@meeseeksdev backport to v5.3.x