Make run record datetime fields in UTC instead of naive datetimes, remove datetime_as_float #22818
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gibsondan
force-pushed
the
runrecorddatetime
branch
5 times, most recently
from
b8980a6
to
49238a4
Compare
gibsondan
commented
Jul 2, 2024
| @@ -271,16 +271,24 @@ def _add_filters_to_query(self, query: SqlAlchemyQuery, filters: RunsFilter) -> | |||
| query = query.where(RunsTable.c.snapshot_id == filters.snapshot_id) | |||
|
|
|||
| if filters.updated_after: | |||
| query = query.where(RunsTable.c.update_timestamp > filters.updated_after) | |||
| query = query.where( | |||
| RunsTable.c.update_timestamp > filters.updated_after.replace(tzinfo=None) | |||
There was a problem hiding this comment.
this is a pre-existing bug that this PR surfaced, if you didn't pass in a naive datetime we weren't passing it in correctly
alangenfeld
reviewed
Jul 2, 2024
Comment on lines
614
to
626
| return self._asdict()["create_timestamp"] | ||
|
|
||
| @property | ||
| def update_time(self) -> datetime: | ||
| return self._asdict()["update_timestamp"] | ||
|
|
||
| @property | ||
| def start_timestamp(self) -> Optional[float]: | ||
| return self._asdict()["start_time"] | ||
|
|
||
| @property | ||
| def end_timestamp(self) -> Optional[float]: | ||
| return self._asdict()["end_time"] |
Comment on lines
612
to
626
| @property | ||
| def create_time(self) -> datetime: | ||
| return self._asdict()["create_timestamp"] | ||
|
|
||
| @property | ||
| def update_time(self) -> datetime: | ||
| return self._asdict()["update_timestamp"] | ||
|
|
||
| @property | ||
| def start_timestamp(self) -> Optional[float]: | ||
| return self._asdict()["start_time"] | ||
|
|
||
| @property | ||
| def end_timestamp(self) -> Optional[float]: | ||
| return self._asdict()["end_time"] |
There was a problem hiding this comment.
there should probably be a comment somewhere in here about this time / timestamp flip flopping
should we like do serdes stuff to "correct" the model to the right way and then add the back compat accessors as deprecated?
gibsondan
force-pushed
the
runrecorddatetime
branch
from
49238a4
to
8785238
Compare
|
Took out the new fields on RunRecord since that's a whole other can of worms |
gibsondan
force-pushed
the
runrecorddatetime
branch
from
8785238
to
53b605c
Compare
alangenfeld
approved these changes
Jul 2, 2024
….datetime_from_timestamp and move utc_datetime_from_naive into dagster._time Summary: Consolidating all the datetime utils into one place.
gibsondan
force-pushed
the
runrecorddatetime
branch
from
53b605c
to
5120be8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This brings run records in OSS in line with run records that are returned over our API in that it has non-naive datetimes - UTC datetimes are strictly better than naive datetimes. It also allows us to remove this datetime_as_float utiliyt method which was designed as a way to reliably handle these naive datetimes coming out of sqlite - my claim is that the new syntax that I have replaced it with (utc_datetime_from_naive(...).timestamp()) is much clearer about what is going on.
Somewhat unfortunate that the field on RunRecord is "update_timestamp" and not "update_time", but that would be a breaking change.
Summary & Motivation
How I Tested These Changes