[Dashboard] Fix timezone_utils to deal with daylight savings#51314
[Dashboard] Fix timezone_utils to deal with daylight savings#51314bhmiller wants to merge 1 commit intoray-project:masterfrom
Conversation
There was a problem hiding this comment.
Did not exist in Intl.DateTimeFormat
There was a problem hiding this comment.
Did not exist in Intl.DateTimeFormat
86909ec to
c346d03
Compare
python/requirements.txt
Outdated
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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.
yeah, I am more of asking the component owner.
There was a problem hiding this comment.
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.
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
There was a problem hiding this comment.
and I am not a fan of this new dependency..
There was a problem hiding this comment.
Can you say more on why? It's already a dependency of yours.
There was a problem hiding this comment.
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.
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/timezonethattzlocaldoes, 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 <6900229+bhmiller@users.noreply.github.com> Co-authored-by: Alan Guo <aguo@anyscale.com>
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 <6900229+bhmiller@users.noreply.github.com>
|
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 Timezoneis 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.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.My testing was just using python code through the repl.