Re-add local pyown usage with local api-key. Refactored OWNApi into … #123

ulno · 2020-04-14T23:45:39Z

…real wrapper that can use local api_key or default to doing web requests to mycroft.

As discussed with @forslund in the forums.

This allows the weather skill to use a personal OWM api_key (at least when set in local settings.json) and if proxy is set to false.

This effort is probably duplicating issue #27.

Building a connector adapter (that itself could use for example mqtt) into this wrapper might also address issues #98, #100, #115

…real wrapper that can use local api_key or default to doing web requests to mycroft.

krisgesling · 2020-04-24T05:13:16Z

I'm wondering if it would be better to have independent classes for each weather service. They could inherit from a parent WeatherAPI class that defines the structure and handles common functions like caching.

This would keep each more readable and maintainable, as well as opening the door to adding other weather services without needing to produce a Weather Channel Skill, Weather Underground Skill, etc, etc.

This is also a lot more work and outside the scope of this PR, but still interested in any thoughts.

krisgesling

Great stuff Ulno, thanks for taking the initiative on this.

I've flagged a few things that we need to look at before this can be merged, and some others are broader questions that don't necessarily need to be addressed in this PR.

krisgesling · 2020-04-24T05:16:42Z

__init__.py

    ''' Wrapper that defaults to the Mycroft cloud proxy so user's don't need
-        to get their own OWM API keys '''
+        to get their own OWM API keys.
+        However it also handles a local OWM api_key.'''


Perhaps could rewrite a little eg

Wrapper for the Open Weather Maps API.
By default the Mycroft cloud proxy will be used so user's don't need an API key.
If an api_key is provided, it will connect directly to the OWM API instead.

Are you changing that or shall I?

You go for it, I'd have to mess up your PR with a random commit from me

krisgesling · 2020-04-24T05:18:07Z

__init__.py

@@ -91,6 +93,12 @@ def __init__(self):
        self.query_cache = {}
        self.location_translations = {}

+        if api_key and not use_proxy:
+            self.owm = OWM(api_key)


I wonder if local_owm or similar would be more descriptive?

yes, I was also thinking about that or calling the other owm owm_wrapper

krisgesling · 2020-04-24T05:19:50Z

__init__.py

@@ -137,6 +145,7 @@ def build_query(self, params):

    def request(self, data):
        """ Caching the responses """
+        # request is not used for local_api key


Not necessary for this PR, but would be good for us to add caching support for local_api users in the future.

should add a TODO here then

krisgesling · 2020-04-24T05:27:36Z

__init__.py

            q = {"lat": lat, "lon": lon}
        else:
            if name in self.location_translations:
                name = self.location_translations[name]
+            LOG.info("searching weather for {}".format(name)) # TODO: remove debug


Best to switch this to debug level or remove before we merge. We aim to keep the info log stream as clean as possible.

oops, wanted to remove that before PR, but maybe keeping it in as debug makes sense? Could even add some more debug logs?

krisgesling · 2020-04-24T05:35:42Z

__init__.py

-        # TODO: Remove lat,lon parameters from the OWMApi()
-        #       methods and implement _at_coords() versions
-        #       instead to make the interfaces compatible
-        #       again.


I interpreted this as splitting the location methods to make the wrapper interfaces consistent with the OWM API. Though I find the current approach cleaner. A single call for location based queries, and lat, lon would always be considered to be more accurate than a general place name so are prioritised.
Just raising it in case anyone wants to argue the other side before we remove the # TODO

I also think having just one call would be more pythonic

krisgesling · 2020-04-24T05:38:38Z

__init__.py

+        # lat,lon parameters are handled by OWMApi() both for
+        # local api key and mycrodt's api


If we agree on this approach, then I don't know that we need this comment here any more.

and would also kill the spelling mistake

krisgesling · 2020-04-24T05:41:36Z

__init__.py

+        # lat,lon parameters are handled by OWMApi() both for
+        # local api key and mycrodt's api
+
+        self.owm = OWMApi( self.settings["api_key"], self.settings['use_proxy'] )


What's the thinking behind two settings? Sometimes people with their own API key may wish to switch on/off the proxy without adding and removing the key?

Also need to add these settings to the settingsmeta.json

You explain that to me, I just took the parameters that were handled before - I think the proxy should be handled somehow much more globally.

Ah I didn't see that it was just un-commenting some code. This was before my time, not sure what the thinking was.

Personally I think the less settings the better unless it's adding real value, so if there was no need for a separate setting I would leave it at just an api_key. If the custom key is present then use a local OWM connection, else use the proxy.

krisgesling · 2020-04-24T05:44:35Z

__init__.py

+            # Try to rewrite name -> replace last blank with comma # TODO: careful this might be language specific -> check
+            p = name.rfind(" ")
+            if p>0:
+                name = name[:p] + "," + name[p+1:]


Is it worth rechecking whether this new name variation exists in the registry?
If not we can perhaps return a location not found error earlier.

Yep, I am not sure how your location registry actually works and what is in there - so maybe there would be a better solution than this.

I don't know much about the registry apart from what is in the pyowm docs

This is a broader problem which is why we added a geolocation endpoint to Mycroft. It's not an easy thing to parse a string and return an exact location. There's always multiple places with the same name, an unknown number of words in each part of the location, and you don't know what combination of parts people are saying eg suburb + country, city + state, city + state + country etc.

Happy to include this in the interim as it does check another variation of possibilities, but ultimately I think we need to move to a complete location parser. Translating the location to coordinates means we can be more confident in the response from OWM.

ulno · 2020-04-24T12:35:53Z

I'm wondering if it would be better to have independent classes for each weather service. They could inherit from a parent WeatherAPI class that defines the structure and handles common functions like caching.

This would keep each more readable and maintainable, as well as opening the door to adding other weather services without needing to produce a Weather Channel Skill, Weather Underground Skill, etc, etc.

This is also a lot more work and outside the scope of this PR, but still interested in any thoughts.

I agree, this should be the long term goal, however, I think we need to stay agile and rather allow ourselves to refactor that at one point when we have enough different usecass.

Talking about sidenotes: it woudl be really nice if we coudl also get some information on precipitation, air pressure, and chance of rain.

ulno added 2 commits April 14, 2020 20:42

Re-add local pyown usage with local api-key. Refactored tOWNApi into …

4ba1d94

…real wrapper that can use local api_key or default to doing web requests to mycroft.

add to deal with city/state combos

1a0d5bf

krisgesling reviewed Apr 24, 2020

View reviewed changes

JarbasAl mentioned this pull request May 12, 2021

Make weather great again #158

Merged

5 tasks

JarbasAl mentioned this pull request Jun 27, 2021

Use own keys #170

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-add local pyown usage with local api-key. Refactored OWNApi into … #123

Re-add local pyown usage with local api-key. Refactored OWNApi into … #123

ulno commented Apr 14, 2020 •

edited

krisgesling commented Apr 24, 2020

krisgesling left a comment

krisgesling Apr 24, 2020

ulno Apr 24, 2020

krisgesling Apr 27, 2020

krisgesling Apr 24, 2020

ulno Apr 24, 2020

krisgesling Apr 24, 2020

ulno Apr 24, 2020

krisgesling Apr 24, 2020

ulno Apr 24, 2020

krisgesling Apr 24, 2020

ulno Apr 24, 2020

krisgesling Apr 24, 2020

ulno Apr 24, 2020

krisgesling Apr 24, 2020

ulno Apr 24, 2020

krisgesling Apr 27, 2020

krisgesling Apr 24, 2020

ulno Apr 24, 2020

krisgesling Apr 27, 2020

ulno commented Apr 24, 2020

		# lat,lon parameters are handled by OWMApi() both for
		# local api key and mycrodt's api

Re-add local pyown usage with local api-key. Refactored OWNApi into … #123

Are you sure you want to change the base?

Re-add local pyown usage with local api-key. Refactored OWNApi into … #123

Conversation

ulno commented Apr 14, 2020 • edited

krisgesling commented Apr 24, 2020

krisgesling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulno commented Apr 24, 2020

ulno commented Apr 14, 2020 •

edited