dagster-io / dagster

An orchestration platform for the development, production, and observation of data assets.
https://dagster.io
Apache License 2.0
10.7k stars 1.33k forks source link

Replace deprecated utcfromtimestamp method #21446

Closed psarka closed 1 month ago

psarka commented 2 months ago

What's the use case?

I'm seeing these warnings:

.../lib/python3.12/site-packages/dagster/_utils/__init__.py:72: DeprecationWarning: datetime.datetime.utcfromtimestamp() is deprecated and scheduled for removal in a future version. Use timezone-aware objects to represent datetimes in UTC: datetime.datetime.fromtimestamp(timestamp, datetime.UTC).
    EPOCH = datetime.datetime.utcfromtimestamp(0)

Ideas of implementation

To my knowledge, the recommended replacement is datetime.datetime.fromtimestamp(0, datetime.UTC).replace(tzinfo=None).

Additional information

utcfromtimestamp is used in a handful of places, probably makes sense to fix all of them

📎 python_modules/dagster$ grep utcfromtimestamp . -rI
./dagster/_core/definitions/data_time.py:        run_failure_time = datetime.datetime.utcfromtimestamp(
./dagster/_core/definitions/data_time.py:            return datetime.datetime.utcfromtimestamp(cast(float, data_time.value)).replace(
./dagster/_core/storage/event_log/sql_event_log.py:                            event_timestamp=datetime.utcfromtimestamp(event_timestamp),
./dagster/_core/storage/event_log/sql_event_log.py:                > datetime.utcfromtimestamp(asset_details.last_wipe_timestamp)
./dagster/_core/storage/event_log/sql_event_log.py:                < datetime.utcfromtimestamp(event_records_filter.before_timestamp)
./dagster/_core/storage/event_log/sql_event_log.py:                > datetime.utcfromtimestamp(event_records_filter.after_timestamp)
./dagster/_core/storage/event_log/sql_event_log.py:                            > datetime.utcfromtimestamp(asset_details.last_wipe_timestamp),
./dagster/_core/storage/event_log/sql_event_log.py:                    > datetime.utcfromtimestamp(asset_details.last_wipe_timestamp)
./dagster/_core/storage/event_log/sql_event_log.py:                    > datetime.utcfromtimestamp(asset_details.last_wipe_timestamp)
./dagster/_utils/__init__.py:EPOCH = datetime.datetime.utcfromtimestamp(0)

Message from the maintainers

Impacted by this issue? Give it a 👍! We factor engagement into prioritization.

dbrtly commented 1 month ago

I can try this one. Not sure what tests if any you want to add.

I notice sometimes we import datetime and then refer to datetime.timezone.utc and at other times, from datetime import timezone and then use timezone.utc. Is the latter preferred?

Looks like there are two patterns needed:

convert unix timestamp at UTC at second precision to iso8601 timestamp at explicit utc of type timestamp.timestamp occurs twice in data_time.py

example: float =  1715478630.0 
before_explicit_tz = datetime.datetime.utcfromtimestamp(cast(float, example)).replace(tzinfo=datetime.timezone.utc)
print("before is", before_explicit_tz, type(before_explicit_tz))

after_explicit_tz = datetime.datetime.fromtimestamp(cast(float, example), datetime.timezone.utc)
print("after is ", after_explicit_tz, type(after_explicit_tz))

convert unix timestamp in UTC at second precision to iso8601 datetime at implicit UTC of type timestamp.timestamp. Occurs 8 times in 3 other files.

example: float =  1715478630.0 
before_implicit_tz = datetime.datetime.utcfromtimestamp(example)
print("before is ", before_implicit_tz, type(before_implicit_tz))

after_implicit_tz = datetime.datetime.fromtimestamp(example, datetime.timezone.utc).replace(tzinfo=None)
print("after is ", after_implicit_tz, type(after_implicit_tz))
Blackskies commented 1 month ago

Hi @psarka is this issue still open to contribute ? and can I give it a try

I am new comer to open source contribution and want to help

garethbrickman commented 1 month ago

@Blackskies Thanks for wanting to contribute but it looks like @dbrtly already merged a PR fixing this https://github.com/dagster-io/dagster/pull/21799 👍