Fixes looking around

[fstagni]

No description provided.

[chaen]

I think this was on purpose, given that there was the proper noqa https://docs.astral.sh/ruff/rules/call-datetime-utcnow/
I would guess it has to do with DIRAC compatibility but I don't remember

[fstagni]

utcnow will be deprecated. What I put is of course equivalent.

[chaen]

Even though it is subtle, it is not

In [19]: tz_now = datetime.now(tz=timezone.utc); now = datetime.utcnow()

In [20]: tz_now
Out[20]: datetime.datetime(2024, 8, 22, 21, 13, 40, 232387, tzinfo=datetime.timezone.utc)

In [21]: now
Out[21]: datetime.datetime(2024, 8, 22, 21, 13, 40, 232433)

In [22]: tz_now - now
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[22], line 1
----> 1 tz_now - now

TypeError: can't subtract offset-naive and offset-aware datetimes

I was tricked by that in DIRAC in the past:

https://github.com/DIRACGrid/DIRAC/blob/f18be6f9d27f5055512f2c372718734270a060ae/src/DIRAC/Core/Security/m2crypto/asn1_utils.py#L196-L201

But indeed here we should check why was this done and maybe your fix is good.

[fstagni]

Further converting it is OK:

In [9]: formatted__tz_now = tz_now.isoformat(sep=' ').split("+")[0]

In [10]: formatted__now = now.isoformat(sep=' ').split("+")[0]

In [14]: formatted__tz_now
Out[14]: '2024-08-23 08:56:13.234331'

In [15]: formatted__now
Out[15]: '2024-08-23 08:56:13.234372'

In [16]: date_obj__tz_now = datetime.strptime(formatted__tz_now, '%Y-%m-%d %H:%M:%S.%f')

In [17]: date_obj__now = datetime.strptime(formatted__now, '%Y-%m-%d %H:%M:%S.%f')

In [18]: date_obj__tz_now -  date_obj__now
Out[18]: datetime.timedelta(days=-1, seconds=86399, microseconds=999959)

But:

In [20]: date_obj__tz_now__no_split = datetime.strptime(formatted__tz_now__no_split, '%Y-%m-%d %H:%M:%S.%f')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[20], line 1
----> 1 date_obj__tz_now__no_split = datetime.strptime(formatted__tz_now__no_split, '%Y-%m-%d %H:%M:%S.%f')

File ~/miniforge3/envs/dirac-development/lib/python3.11/_strptime.py:567, in _strptime_datetime(cls, data_string, format)
    564 def _strptime_datetime(cls, data_string, format="%a %b %d %H:%M:%S %Y"):
    565     """Return a class cls instance based on the input string and the
    566     format string."""
--> 567     tt, fraction, gmtoff_fraction = _strptime(data_string, format)
    568     tzname, gmtoff = tt[-2:]
    569     args = tt[:6] + (fraction,)

File ~/miniforge3/envs/dirac-development/lib/python3.11/_strptime.py:352, in _strptime(data_string, format)
    349     raise ValueError("time data %r does not match format %r" %
    350                      (data_string, format))
    351 if len(data_string) != found.end():
--> 352     raise ValueError("unconverted data remains: %s" %
    353                       data_string[found.end():])
    355 iso_year = year = None
    356 month = day = 1

ValueError: unconverted data remains: +00:00

In [21]: formatted__tz_now__no_split
Out[21]: '2024-08-23 08:56:13.234331+00:00'

In [22]: date_obj__tz_now__no_split = datetime.strptime(formatted__tz_now__no_split, '%Y-%m-%d %H:%M:%S.%f+00:00')

In [23]:  date_obj__tz_now__no_split
Out[23]: datetime.datetime(2024, 8, 23, 8, 56, 13, 234331)

In [24]: date_obj__tz_now__no_split -  date_obj__now
Out[24]: datetime.timedelta(days=-1, seconds=86399, microseconds=999959)

...I guess it is a feature, but to me it even feels like a python bug! Bof!

[chaen]

Why removing the __future__ import ?

[fstagni]

Why removing the __future__ import ?

Because since python 3.7 is only necessary for specific cases.

[chrisburr]

Because since python 3.7 is only necessary for specific cases.

It was added in 3.7 so that can't be the case 😉

The intention is to make the from __future__ import annotations the default (see PEP-563 but it keeps getting deffered.

[chaen]

It actually should be everywhere except specific places like

diracx/diracx-routers/src/diracx/routers/access_policies.py

Lines 31 to 35 in b86ed11

	# FastAPI bug:
	# We normally would use `from __future__ import annotations`
	# but a bug in FastAPI prevents us from doing so
	# https://github.com/tiangolo/fastapi/pull/11355
	# Until it is merged, we can work around it by using strings.

[fstagni]

Sorry, I meant 3.10, but indeed I realise now that the decision to make it mandatory was postponed and it is not even in 3.12. So, I revert this change.

[chrisburr]

Can we avoid making the tests slower by using elliptic curves?

[fstagni]

In this PR? Can we leave for another one?

[chaen]

it's not this environment.yml but the one from container-images that should be used

[fstagni]

This environment.yml defines the environment where all of us are working locally, right? So, it's the natural environment where all unit tests run.

In container-images there are many environment.yml files. What is here is, permit me that, the superset of all of them.

[chaen]

absolutely, but this is what we want to try to avoid. We want to have the minimal set of dependencies installed to keep control of what we actually need, and this is why we did not blindly put the environment.yml before

[fstagni]

So, you want to repeat, for ONLY the unit tests run by the CI, the ~same environment (because it is not going to be very different) that we all have in our local machines, and that we all use for running the unit tests locally? Meaning that I can run the unit tests locally, they all pass, then create the PR only to discover that there they are failing. If there's a version to restrict (like for the one for typer), that, like it or not, we have to do it from time to time, then we would need to apply that fix in even one more place.

There is already a noted duplication between setup.cfg and environment.yml and I do not think we should have even more.

The environment where the unit tests run is already NOT what it is run in production. Which is fair and fine. But that is why the integrations tests are there.

[chrisburr]

We expect the CI to run in an environment which is different to out local machines as we intetionally only install the bare minimum in the CI. We also let it use the newest possible version to try and catch issues before they affect the harder/slower to debug tests.

Maybe we want to drop the environment.yaml file entirely but that's a separate discussion to start and maybe we want to use something else to harmonise this but we've not got around to trying it.

[chrisburr]

We have discussed this before...

[chrisburr]

#61 (comment)

[fstagni]

Superseded by #286