improved skill settings #1151
improved skill settings #1151
Conversation
added capability to auto upload changes from settingsmeta.json to home.mycroft.ai
==== Documentation Notes ====
If a developer make changes to the settingsmeta.json, then this will be auto uploaded to home.mycroft.ai
==== Protocol Notes ====
hash and uuid are now stored as variables in files located in ~/.mycroft/skills/{skill-name}
mycroft/skills/settings.py
Outdated
| self._save_uuid(new_uuid) | ||
| self._save_hash(hashed_meta) | ||
| else: # if hash is old | ||
| should_exist_in_backend = False |
There was a problem hiding this comment.
Would it be clearer to rename to something like found_in_backend?
mycroft/skills/settings.py
Outdated
| @@ -74,80 +106,154 @@ def _is_stored(self): | |||
| return hash(str(self)) == self.loaded_hash | |||
|
|
|||
| def __getitem__(self, key): | |||
| """ Get key | |||
| """ | |||
There was a problem hiding this comment.
How about """ Get key """ on one line? (Same below)
| sections = skill_setting['skillMetadata']['sections'] | ||
| for section in sections: | ||
| for field in section["fields"]: | ||
| self.__setitem__(field["name"], field["value"]) |
There was a problem hiding this comment.
Not sure, but would it make sense to write it as:
self[field['name']] = field['value']
mycroft/skills/settings.py
Outdated
| @@ -161,17 +267,15 @@ def load_skill_settings(self): | |||
| LOG.error(e) | |||
|
|
|||
| def _get_settings(self): | |||
There was a problem hiding this comment.
This hasn't been changed in this PR, but should we rename this to _get_remote_settings for clarity?
|
I've just skimmed the code so I might be totally off but won't the Timers hang around after skill shutdown? endlessly repeat? each time a skill is restarted a new one will occur? Also the tail reactivated timer is quite unreliable when trying to shutdown since cancel won't work while the timed function is executed. I'll look in more detail tomorrow morning. |
penrods
added
the
CLA: Yes
|
Did some basic testing and the timers are hanging around after settings goes out of scope. This means each time a skill get's reloaded another timer will be started. The timers also hinders the process to exit cleanly, This can be quickly fix by setting the |
|
@forslund @MatthewScholefield fixed the timer issue. can you review |
|
This solves part of the problem. This will allow the skill process to terminate despite timers being active. However the Timer still seems to keep going after skill shutdown as far as I can see (and I might very well be missing something). Probably not a big deal in most cases but not a good situation in the long run. I have an idea to deal with this using weakrefs but won't have time to deal with this tonight. I can get up early tomorrow morning and give it a go but if you think this is acceptable for this release go ahead and merge. |
|
@forslund I'm pretty sure this won't continue after restarting the skills service because this example python code will immediately exit without printing #!/usr/bin/env python2
from __future__ import print_function
from threading import Timer
t = Timer(1, print, args=('hello',))
t.daemon = True
t.start() |
|
Not restarting the process, that is all good. Reloading the skill. This happens when a skill is changed for example at an update. |
|
@forslund matthew came up with an idea to set a flag in skill settings called is_alive, and we only poll if is_alive = True, on shutdown in the core.py we can call self.settings.is_alive = False, and that should stop the polling. |
|
That will work just fine. (I really just wanted a reason to use weakrefs :) ) Add that and merge! |
improved skill settings
==== Tech Notes ====
added capability to auto upload changes from settingsmeta.json to home.mycroft.ai
==== Documentation Notes ====
If a developer make changes to the settingsmeta.json, then this will be auto uploaded to home.mycroft.ai
NOTE: currently the auto delete of previous settingsmeta.json from device function is commented out until the api end point is implemented in the backend. For now once you make changes to the settingsmeta.json, you will have an entirely new entry in home.mycroft.ai. You will need to delete the old one manually on home.mycroft.ai.
==== Protocol Notes ====
hash and uuid are now stored as variables in files located in ~/.mycroft/skills/{skill-name}