-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[Dashboard] Fix timezone_utils to deal with daylight savings #51314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
value: "America/Chihuahua", | ||
group: "America", | ||
country: "Chihuahua", | ||
}, | ||
{ | ||
utc: "GMT-06:00", | ||
value: "America/Guadalajara", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not exist in Intl.DateTimeFormat
value: "Asia/Bangkok", | ||
group: "Asia", | ||
country: "Bangkok", | ||
}, | ||
{ | ||
utc: "GMT+07:00", | ||
value: "Asia/Hanoi", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not exist in Intl.DateTimeFormat
86909ec
to
c346d03
Compare
python/requirements.txt
Outdated
@@ -62,3 +62,4 @@ py-spy>=0.2.0; python_version < '3.12' | |||
py-spy>=0.4.0; python_version >= '3.12' | |||
memray; sys_platform != "win32" # memray is not supported on Windows | |||
pyOpenSSL | |||
tzlocal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to get this available for your tests. When I ssh-ed into ray head, I saw tzlocal
, so I think it's available normally, but your tests were failing with not being able to find tzlocal
. I could use some help pointing to how to get the tests happy as I'm otherwise just skimming through the codebase and guessing.
a886e2e
to
dd4ca51
Compare
(tz for tz in timezones if tz["offset"] == current_offset), | ||
{"offset": None, "value": None}, | ||
) | ||
current_timezone = {"offset": current_offset, "value": tzlocal.get_localzone_name()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this get_current_timezone_info
is required in the first place? for frontend ui, it should either use browser time or use utc, right? why does server side local timezone even matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not implement this feature of yours. I'm trying to fix that the feature doesn't work. If you would rather drop the server timezone and just use the browser's time zone, that's fine by me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I am more of asking the component owner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default option is to use server timestamp because that is the timestamp that is printed to the log files.
We provide an option to use the browser timestamp for convenience, but that will not update the timestamp already printed to logs, which will be confusing.
The Ray Dashboard does not parse any logs today. It just prints them as raw text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the simplest approach would be if we used UTC everywhere, but I'm not sure if that's the behavior everyone wants.
A quick fix to the issue at hand would be to drop the timezone names from the UI and only show offset numbers in the dropdown.
python/setup.py
Outdated
@@ -264,6 +264,7 @@ def get_packages(self): | |||
pydantic_dep, | |||
"prometheus_client >= 0.7.1", | |||
"smart_open", | |||
"tzlocal >= 5.3", # Required for Ray dashboard timezone handling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and I am not a fan of this new dependency..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you say more on why? It's already a dependency of yours.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not ray dependency, it is a dependency of anyscale CLI, which is currently required to be built into the ray release test image to run ray release tests, which is why we added in the dependency resolving procedure and pinning the version for CI/CD. ray does not depend on tzlocal
. if you pip install ray[all]
on an empty python virtual env today, tzlocal
will not be installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, if you don't want to use tzlocal other options I see:
- Drop server time (your previous question). Note in terms of use cases, a remote worker in a different time zone might like using server time zone (though I'm sure that remote worker experiences issues related to this in 200 other places besides the ray dashboard).
- Shortcut it and just use
os.environ["TZ"]
(which skips the full resolution of going to/etc/timezone
thattzlocal
does, as I don't want to duplicate what an off the shelf thing does). - Try and mirror what the typescript side does - e.g. have a "preferred" list of IANA tiime zones to deal with the lack of 1:1 mapping in python abbreviations:
from datetime import datetime
from zoneinfo import ZoneInfo, available_timezones
now = datetime.now()
timezone_map = {tz: now.astimezone(ZoneInfo(tz)).tzname() for tz in available_timezones()}
tz_abbreviation = now.astimezone().tzname()
possible_timezones = [timezone for timezone, abbreviation in timezone_map.items() if abbreviation == tz_abbreviation]
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.
|
Note I split out the front end change into a separate PR as have gotten no traction on this PR given the questions around what to do on the python side. |
Did not get traction with the [holistic fix](#51314) for daylight savings as there was pushback on usage of tzlocal in python. So, splitting out the fix on the UI side which is perhaps less controversial. On the UI side, `timezone.ts` is showing GMT offsets not reflective of daylight savings time. Updating to dynamically pull the offset from `Intl.DateTimeFormat`. Note not doing any dynamic evaluation so for browser windows open during the switch, it will not update. --------- Signed-off-by: Brendan Miller <[email protected]> Co-authored-by: Alan Guo <[email protected]>
The `Dashboard Server Timezone` is currently incorrect in when running in a timezone with daylight savings. Fixing that resolution with python `tzlocal`. Signed-off-by: Brendan Miller <[email protected]>
Rebased this on top of my merged PR in #52755 . So now all that's left in this change is the python changes to fix server time zone. I backed out my attempts to get testing references happy, so expect this to mostly just fail, but I'm otherwise not working on the PR given the feedback/no consensus on how ray folks want to solve this bug (didn't like tzlocal). |
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
This pull request has been automatically closed because there has been no more activity in the 14 days Please feel free to reopen or open a new pull request if you'd still like this to be addressed. Again, you can always ask for help on our discussion forum or Ray's public slack channel. Thanks again for your contribution! |
The
Dashboard Server Timezone
is currently incorrect in whenrunning in a timezone with daylight savings. Fixing that
resolution with python
tzlocal
.Why are these changes needed?
See #51310
Related issue number
Closes #51310
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.My testing was just using python code through the repl.