skill loading requirements continued #272
Conversation
|
I'm sure I missed something in chat, but what is the intent here? Are there skills that need to load after the GUI, or skills that shouldn't load unless there's a GUI? |
follow up to #245 in chat i realized that some skills should only load (and only if) gui is connected, main example being homescreen. @AIIX asked to remove it from requirements for now since some platforms dont have GUI, i figured this approach makes more sense as our images should just work without needing a specific one per platform and we want to pin skill versions for all default skills. more examples would be OCP movie skills, if you cant play movies why load them at all just to slow things down? this is an optimization, and skills would need to explicitly request to have gui ready before loading, nothing changes by default |
|
this is essentially a way for skills to say "I didn't implement a voice interaction"/"My purpose is intrinsically tied to a GUI" Counter argument, skills are all about voice, if it does not have a VUI it should not be a skill but live in PHAL maybe ? ie, homescreen skill should be homescreen-PHAL-plugin |
|
one more example, "hey mycroft, show me a picture" current - each skill implements |
Codecov Report
@@ Coverage Diff @@
## dev #272 +/- ##
==========================================
+ Coverage 50.35% 52.26% +1.91%
==========================================
Files 119 156 +37
Lines 10077 8265 -1812
==========================================
- Hits 5074 4320 -754
+ Misses 5003 3945 -1058
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Could this be an optional setting, o be able to override? Example would be with OCP to send its output to the living room tv. |
|
From the chat if recall this is fully optional and not a hard requirements for GUI skills to incorporate. Also would like some clarity on this:
In both the above scenarios GUI is never pre-connected or embedded in the shell (unlike bigscreen), GUI works as standalone applications in both the above targets. Cons of this approach: GUI skills loading is blocked until the application is opened Expected behavior: Skills providing GUI are already ready and loaded for use when user opens application on Mobile / Desktop without having to wait on skill loader to only load GUI skills after a GUI has connected I would prefer this implementation to be fully optional put behind some flag in configuration, Every platform should be able to implement this optionally (rather than it being forced and slowing down GUI skills from loading and being ready for use) Skills that handle "if gui X else" can also handle the intent fallback if GUI is not available, a simple implementation here would be for a method to forward the failed intent to a fallback skill that skill authors can implement |
|
Example on plasma mobile: "Hey Mycroft, show me the recipe for xyz" With this implementation when the GUI application is not opened "I cannot show you the recipe" or some fallback |
this is not a Con, this is literally the point, the feature being implemented, but applies only to skills that request it, not by default If we added it to recipes: "Hey Mycroft, show me the recipe for xyz"
so while i dont think recipes is a good example for this, it also would be nice, you mention user experience, im exactly thinking about VOICE user experience, this reduces intent collision and some packaging quirks (such as allowing to bundle skills without messing with headless installs) but again, a skill needs to ask for this to happen, nothing changes for regular skills, dont like the feature dont use it in your skills, simple
does not make sense, this is per skill, each skill dev is already sending "some flag" for the specific skill, this is not a global thing check the RuntimeRequirements class, see default values unless a skill changes it, the comments there should help understanding too class RuntimeRequirements:
# xxx_before_load is used by skills service
network_before_load: bool = True
internet_before_load: bool = True
gui_before_load: bool = False
# requires_xxx is currently purely informative and not consumed by core
# this allows a skill to spec if it needs connectivity to handle utterances
requires_internet: bool = True
requires_network: bool = True
requires_gui: bool = False
# xxx_fallback is currently purely informative and not consumed by core
# this allows a skill to spec if it has a fallback for temporary offline events, eg, by having a cache
no_internet_fallback: bool = False
no_network_fallback: bool = False
no_gui_fallback: bool = True # can work voice only |
ChatGPT is not the end of the world, I disagree with this, This is simply a method for killing the GUI Experience on a platform and I am exactly thinking about the GUI USER Experience on all the mentioned platforms. This approach simply limits the GUI experience on any platform and causes unrequested problems for GUI clients where they work as applications and are not started at boot time. You previously provided an example using "Show Picture" and ChatGPT is surely not going to speak a Picture out. With the recipes skill its still clearly very much possible for the Recipes skill to provide a Voice Interaface without needing the GUI to display, this also means the skill is very much Loaded and Ready for use. ChatGPT fallback is not needed at all here when the skill is enough capable of providing the voice experience.
All platforms depend on default skills provided by OVOS, so are you implying that all default GUI providing skills should be moved away from OVOS because OVOS is going to now force this implementation ? Sure as an individual skill author you can feel free to implement it for your own skills if you wish, but i do not agree on this implementation for default OVOS skills that will be used on all platforms and negatively impact the GUI experience for those or any platforms.
It make a lot of sense for platforms where this should be fully disabled or ignored, it doesn't matter if the skill implements it or not, the platform must be able to decide that if its running an app like environment where skills should be preloaded and not provide a bad GUI experience waiting for GUI skills to load each time the application opens and closes. |
|
if we have a gui -> select the gui skill (supposedly the best way to help user) chatGPT was just an example that could answer recipes.... i used fallback-unknown also as an example earlier to illustrate a different use case, the point here is to allow better matching by giving an opportunity to common_qa and stuff like that, instead of forcing an error by capturing the utterance as for the rest, i implied no such thing... each specific skill is what sends this flag, what does that have to do with moving ovos skills away???? this is a new feature, does not impact existing skills, not sure where this misunderstanding is coming from? this has no impact in anything, it just allows a NEW mechanism if skill devs want to use it, i directly gave you an example of wanting to use it in some OCP skills too are you against giving skills a way to say "I do not want to load before the gui connects"? nothing says that by default, a skill needs to request it, only the skill author really knows if the skill needs or wants this, its not a global feature and certainly not being forced on anything |
OCP on platforms where GUI client is not running but available can easily request the platform to show the GUI, and start the video Playback. Even if the GUI is not installed it can always open a third party application to display the video, but that is not the point, every author / platform might require a different behavior for a good user experience.
I am not against it or improving the voice experience, but I am against downgrading the GUI experience to improve something that can be done in a better way, I am just saying make it optional globally for even skills implementing this on platforms that require this mechanism to be fully disabled, so the GUI experience is not affected by this. so in simple terms if there is a flag in global mycroft conf that says |
Then they don't need to do anything. I think you can ignore this completely for platforms where it isn't relevant, or where the implementer has provided a different solution. I'm not in a position to test, but I bet you could check this branch out underneath a running system and nothing would change. Unless I've completely misunderstood, this behavior still needs to be consumed by compliant skills, and there aren't any yet. Indeed, if you check this out under OVOS or Bigscreen and something changes, that'd be a failing review What this offers, as skills come into compliance, is zero-effort portability. Loading skills that can't work in the current environment affects perf twice, first when you allocate a bunch of memory you don't need and then again when you hit the intent parser for intents you can't execute. As skills update to consume this, they'll cease to consume those resources even when they're present. That means Tinker Toys and potatoes can use the same packages and requirements files, whether or not they have a GUI. I think it's a big improvement for that teddy bear, wherever that thing is and God bless. It costs most of us nothing, costs Jarbas and Daniel some refactoring, and new skills can start out this way. |
You are still missing the point and the details here. This implementation depends on the GUI client connecting and disconnecting each time to load GUI Skills, which is not the expected behavior on platforms like plasma-mobile and android where there is only an application running. This results in a degraded user experience every time the application is opened and the skill loader has to load the GUI skills that implement this. This has nothing to do with how OVOS or Bigscreen implements their GUI.
Your argument about zero-effort portability is irrelevant because if you're going to run this on a toaster or on literal potatoes, you don't even need to load the GUI service or install any GUI skills in the first place. I don't care about how you want to run OVOS on your potatoes, but I sure don't want to experience OVOS skills and GUI not working properly on already established platforms like plasma mobile and the desktop using the GUI clients that require all skills to be loaded without this check. The solution is simple: introduce a configuration value that these platforms can use to disable this check. |
|
what do those supposed platforms do differently exactly? I suggest you test this PR directly in those platforms and see if there is an actual issue, then report back with more details
I dont understand what you mean by application here, skills still only load once on boot? |
The Platforms here are mobile and desktop, they start OVOS in the background, they don't automatically start the GUI, the GUI client is an application here for example "Mycroft-GUI-App" just like any normal mobile / desktop application. The GUI Application in this regard will never connect to the OVOS GUI service on boot, and only connect if a user opens the application and close the connection once the app has exited. This means GUI skills implementing this mechanism will never be ready unless the GUI application has opened at-least once on these platforms. I will test this PR as suggested and also prove what I am saying when it comes to these platforms, I don't agree on merging this until it has been tested on all platforms and proper solution has been addressed to the problem. |
Skill.log GUI.log There is no actual client connected at all to the GUI service here |
|
will see if i can replicate what happened in your logs those platforms you mention, what gui extension would they use? I'd rather make this a per extension thing then, if it is platform specific like extensions then should be tied together |
i think the issue is that the property has not been renamed yet, only the object, so you are doing @classproperty
def runtime_requirements(self):but core expects @classproperty
def network_requirements(self):so you are actually testing default values (no require gui) the companion PR in workshop has not been merged yet (not yet ready either) OpenVoiceOS/OVOS-workshop#49 |
JarbasAl
changed the title
|
Seems now unloading is a thing as well, so skills are going to constantly load unload on the desktop each time the plasmoid or application is opened and closed ? |
|
I was thinking about that case earlier. That definitely needs to be configurable. Depending on the user, it will seem entirely obvious that unloading on desktop should work one way or the other in general, and then when you account for the widget 🤕 |
|
unloading was missing from original PR but just a missing feature this is configurable per gui extension and per skill, mobile extension has all this disabled as discussed previously the main case for gui unloading will be desktops, the same rationale for needing it in first place applies if you close the gui, and the desktop is probably the only platform where this will happen often, in a smartspeaker or bigscreen it would indicate a critical failure I assume perhaps smartspeaker and bigscreen extensions should have the default |
|
In some of the comments above it was stated, that the loading will happen only once when any GUI client is connected there was nothing mentioned about constant loading and unloading, yes this should be "permanent=true" for smartspeaker and bigscreen The behaviour is understandable on for desktop applications and gnome and others. But on plasma desktop this constant loading / unloading, (instead of detecting client once and keeping stuff loaded) is likely going to causes a very frustrating user experience for anyone using using KDE plasmoid with ovos-core, even if they have a 32 thread CPU system with unlimited ram. If this is going to be the default behaviour for desktop, i will probably be adding a notice or warning in the GUI documents that KDE Plasmoid will be affected by the loading / unloading behaviour as certain skills will now utilize this optional mechanism when installing them on ovos-core version 0.0.7 onward |
|
I think i will just add an another extension for the Plasmoid once this is merged that disables the unloading part once the plasmoid is detected as installed, don't need to add any notice then, the plasmoid will ship with its on mycroft.conf with the plasmoid extension pre selected. Once the plasmoid is activated by the user all the gui skills can be loaded, and remain active until the next reboot. This way rest of the generic desktop behaviour remains untouched and can follow what the above requirement is, for skills that implement this. |
Still requires this change |
follow up to #245
requires OpenVoiceOS/OVOS-workshop#49