sunset (and i would guess other timedates) wrongly adding tz about skill-weather HOT 13 CLOSED

A test about the dt of sunrise confirms the claim (timestamp:1624331974, convert: 22.6.2021, 05:19:34, utterance: sieben Uhr neunzehn (7:19))

from skill-weather.

i would check this (resp. this) on several dates

from skill-weather.

positively tested (on sunrise/sunset) with

def convert_to_local_datetime(timestamp: time, timezone: str) -> datetime:
    """Convert a timestamp to a datetime object in the requested timezone.

    This function assumes it is passed a timestamp in the UTC timezone.  It
    then adjusts the datetime to match the specified timezone.

    Args:
        timestamp: seconds since epoch
        timezone: the timezone requested by the user

    Returns:
        A datetime in the passed timezone based on the passed timestamp
    """

    return datetime.fromtimestamp(timestamp).replace(tzinfo=pytz.timezone(timezone))

the question is now if the timestamps are in general tz aware

from skill-weather.

when you do datetime.now() or any other "automatically obtained" date, ie, you are not building a datetime object yourself, the timezone is tzlocal() not UTC, those conversions back and forth are causing issues

the same assumption caused a bug in LF, in general i dont think the skill should do this sort of thing at all, this is what lingua_franca is for

from skill-weather.

i dont understand the comment. the timestamp is directly taken from the api dict, so no back and forth is done here.
there is nothing replaced, only added (nothing other than the datetime is built atm, just without a addition/subtraction of tz on top of a timestamp already internalized the tz)

from skill-weather.

i dont understand the comment. the timestamp is directly taken from the api dict, so no back and forth is done here.
there is nothing replaced, only added (nothing other than the datetime is built atm, just without a addition/subtraction of tz on top of a timestamp already internalized the tz)

apologies, disregard my comment then, i didn't realize the timestamp originated from the api, i thought it came from the python modules and the docstrings tripped me up.

Very similar to the other issue i ran into and i wrongly assumed it was the same thing

Do i understand correctly that the API returns the timestamp in the timezone for lat, lon of the request? or is the timezone determined based on other things (ip address ?)

from skill-weather.

Do i understand correctly that the API returns the timestamp in the timezone for lat, lon of the request?

this is how i see it. There are several timestamps scattered over the response but it would make little sense if one was handled differently as the other.

from skill-weather.

Hm... Just fixed the location problem and searched for the sunset in Porto. It is one hour off (the tz-delta between me and porto).
dang, the timestamp is based on my tz not from the location i ask for. That's a bit confusing.

So, to summarize, if you search without a location you don't need to add anything. But aslong as you search with a location the delta must be computed

from skill-weather.

These two tests are consistently failing around this time of day too:

Failing scenarios:
  features/weather-location.feature:10  User asks for the current weather in a location -- @1.1 what is the current local weather in a location
  features/weather-location.feature:13  User asks for the current weather in a location -- @1.4 what is the current local weather in a location

See:

• https://hudson.mycroft.team/blue/organizations/jenkins/mycroft-skills/detail/PR-1534/2/pipeline
• https://hudson.mycroft.team/blue/organizations/jenkins/mycroft-skills/detail/PR-1535/1/pipeline/

from skill-weather.

The test are passing now, possibly as a result of the refactor. is this still an issue?

from skill-weather.

I noticed that Picroft installation does not set up your timezone to your local timezone. If you run timedatectl on a picroft install the timezone is is set to GMT timezone (I think because I changed it a while back to my local timezone). Picroft should set the OS timezone to your local timezone (perhaps when you first run mycroft-setup-wizard) so that you can use datettime.now() to get the current date time with the correct timezone without additional development having to roll out your own datetime handlers such as convert_to_local_datetime(). This could become even more problematic in the future when you have to manipulate date time and perform complex comparisons.

Upon examination, the inital raw data that gets pulled down is initially in UTC but immediately converted to local time using convert_to_local_datetime(), then along the chain there appears to be some back and forth going on and you end up "hearing" the wrong time. So as a workaround, I tweaked the weather.py and dialog.py to keep the sunrise and sunset times in its raw UTC format without needing convert_to_local_datetime() or now_local()

Tweak in weather.py:

from datetime import datetime as dt
...

class CurrentWeather(Weather):
    """Data representation of the current weather returned by the API"""

    def __init__(self, weather: dict, timezone: str):
        super().__init__(weather, timezone)
        self.sunrise = dt.fromtimestamp(weather["sunrise"]) #convert_to_local_datetime(weather["sunrise"],timezone)
        self.sunset = dt.fromtimestamp(weather["sunset"]) #convert_to_local_datetime(weather["sunset"],timezone)
        self.temperature = round(weather["temp"])
        self.visibility = weather["visibility"]
        self.low_temperature = None
        self.high_temperature = None

Tweak in dialog.py:

from datetime import datetime as dt
...
    def build_sunrise_dialog(self):
        """Build the components necessary to speak the sunrise time."""
        if self.intent_data.location is None:
            now = datetime.now() # not using now_local()
        else:
            now = now_local(tz=self.intent_data.geolocation["timezone"])
        if now < self.weather.sunrise:
            self.name += "-sunrise-future"
        else:
            self.name += "-sunrise-past"
        self.data = dict(time=nice_time(self.weather.sunrise))
        self._add_location()

    def build_sunset_dialog(self):
        """Build the components necessary to speak the sunset time."""
        if self.intent_data.location is None:
            now = datetime.now()  # not using now_local()
        else:
            now = now_local(tz=self.intent_data.geolocation["timezone"])
        if now < self.weather.sunset:
            self.name += "-sunset-future"
        else:
            self.name = "-sunset-past"
        self.data = dict(time=nice_time(self.weather.sunset))
        self._add_location()

from skill-weather.

Hey there,

The challenge is that we can't guarantee the system timezone matches the mycroft configured location on all systems. So whilst we could do it for dedicated images like Picroft or the Mark 1 and Mark II distributions - we don't want to be editing system clocks on random devices like peoples desktops.

This is why we have functions like now_local(). They assume that the system clock is correct but pass in the Mycroft configured timezone.

I did just give this a go and realised we have a bug in pointing to the right dialog file. But the times are being converted correctly to my local timezone. Is that what is failing for you, or do you actually want them reported in UTC?

from skill-weather.

Hmm, just went and fixed the dialog file naming issue and am also seeing this timezone issue now

from skill-weather.