Closed swheaton closed 2 months ago
The recent updates focus on refining the handling of date and timezone data within the FiftyOne framework. Changes involve simplifying datetime to date conversion and streamlining timezone checks in the database logic.
File Path | Change Summary |
---|---|
.../core/fields.py |
Simplified datetime to date conversion by removing UTC conversion in to_python method. |
.../core/odm/database.py |
Modified timezone handling by removing specific checks for "utc" and ensuring logic handles unset zones. |
.../unittests/dataset_tests.py |
Added import for time module and changed system time zone temporarily to "Europe/Madrid" for testing. |
Objective | Addressed | Explanation |
---|---|---|
Correct DateField value from MongoDB (#4365) | ❌ | The changes do not directly address the issue with DateField values being incorrectly loaded, which might be related to timezone handling not specifically revised for MongoDB data retrieval. |
In the land of code where the data hops around,
A rabbit tweaked the time, so no bug is found.
Simplify and prune, make the codebase light,
Each line refined, each function right.
🌟🐇 Happy coding, let the dates align! 🌟
- CodeRabbit
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
LGTM. I have no explanation/defense for the comment in
DateField.to_python()
. Apparently I was pretty sure of myself at the time but it doesn't make sense to me now, either 🤷The key thing is that dates always need to be stored in the DB as UTC
Hmm I see. Well I think what is intended then is replace()
? From datetime docs. You want to just assume the date is in UTC in DB, ignoring whatever timezone options were applied to the DB. I think this jives with the original comment and the (now-failing) unit test.
If you merely want to attach a time zone object tz to a datetime dt without adjustment of date and time data, use dt.replace(tzinfo=tz).
@brimoor i think i fixed it for real this time and added an exposing test case by mocking the "local" timezone.
What changes are proposed in this pull request?
Closes #4365
When the DB is not made timezone aware, it is not UTC by default but rather timezone agnostic. So when you call
astimezone(pytz.utc)
it assumes local timezone in order to do the conversion. When timezone is GMT-2, the stored date (2023, 9, 8, 0, 0) becomes (2023, 9, 8, 2, 0) which results in the same date so you don't see a difference. When timezone is GMT+2, like the reporting user, (2023, 9, 8, 0, 0) becomes (2023, 9, 7, 22, 0) which is a different date (2023/09/07) than expected!db.with_options()
to apply timezone awareness of "UTC" if that is specified info.config
. A datetime with timezone UTC is not the same as a datetime with no timezone.Date
field which will do the wrong thing in the case that the datetime is not timezone aware. I don't know why we wanted to convert to UTC in the first place. See inline PR comments.How is this patch tested? If it is not, please explain why.
I am not sure how to test because you have to fake localized timezone. There's probably a way to do that.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
What areas of FiftyOne does this PR affect?
fiftyone
Python library changesSummary by CodeRabbit