Prioritise remote user config over device #2861
Conversation
krisgesling
added
Type: Enhancement - proposed
devops-mycroft
added
the
CLA: Yes
|
How does this affect values set for a device in /etc/mycroft/mycroft.conf at image creation time, does this now mean that if i have for example a "platform" key value or "recording_timeout" key value set in /etc/mycroft/mycroft.conf it will now get overridden every time by default values from home.mycroft.ai every time mycroft.conf is synced from the remote backend ? Also the whole point of having the SYSTEM override default values is that any platform shipping mycroft should be able to provide their own values via system expected global path than having to depend on USER override which Linux packaging debs/rpm/etc cannot anyways write to. |
|
It's roundabout, but the system could first check device settings for something like |
|
200% disagree with changing the order. The order as it is in now is the only logical order.
I understand that 2 and 3 could bite each other in certain cases, but then it means it's needs a problem solving technique, not such a breaking change as "quick fix". There is a reason, the systemwide config file resides in /etc which is only writable by the root user. If the remote config is moved downwards in that list, it should only override/set variables not already set within /etc/mycroft and ~/.mycroft Again, there is a reason variables might be set in either of the local files. |
|
I think it might be helpful to describe the different levels in more detail, at least as I see them:
In this fashion we move from:
To:
So yes, this would mean that anything set in a remote backend like home.mycroft.ai would override the SYSTEM config. If the backend pushed a As it stands if you set a default TTS engine or wake word configuration for a device then the user can no longer change this via the backend. An alternative would be to add in another level so you had:
I just haven't seen the use case for the SYSTEM_OVERRIDES, generally the two configs just avoid each other, hence I thought switching was the cleaner option. Are there examples of when we would want a SYSTEM config that is set by a device admin to override REMOTE that is set by the end user? Or are there different use cases I'm not thinking about? |
As I see it, Shouldn't any change made by the user in the remote backend ideally be stored at USER level override that being in ~/.mycroft/mycroft.conf rather than changing the logical order of configurations ? Because it seems more logical that any change made by the user is going to override every other configuration default / system for any field be that is coming from the backend or somewhere from a skill. Any changes a user makes to ~/.mycroft/mycroft.conf gets synced to the backend and any changes that are made in the backend by the "USER" should only get synced back to ~/.mycroft/mycroft.conf, otherwise what is the use of ~/.mycroft/mycroft.conf USER override without this not already being the case ? |
|
My point of view; 1 ) DEFAULT - set in mycroft-core, default for every installation of mycroft, can only be changed with a PR or intentionally monkey patching it. Above is to make sure all values are set. In my view this should not be changed as every variable needs to be set and at development time, a proper default value is choosen.
As example: OpenVoiceOS ships with the below configuration; 3 & 4) REMOTE - set if pairing with a backend and updates as the user modifies settings via the web backend. | USER - can be modified by any authenticated user on the device. The last two options again should perfectly sync and work both ways. I think we say the same @krisgesling It is just that the LOCAL and REMOTE architecture should be changed, so that you can change setting either way; The backend, the mycroft-config cli command or manually editting the user mycroft.conf |
|
I like the idea of combining USER and REMOTE. Both are user-level overrides to settings provided by SYSTEM and DEFAULT configs. In fact, I was bitten recently by the USER settings overriding what I had set in Selene. This is a bigger change that can be implemented in the future, but swapping the config order, as this PR does, is essentially the same thing, as long as you don't screw yourself in the USER configs. If there are any configs in SYSTEM or DEFAULT that should be protected from user overrides, such as "platform", perhaps a configuration file is the wrong place for them. Why put something in a configuration file if it is not configurable? |
I believe the whole idea of System Level Override was that any project is free to override the default configuration provided by Mycroft as seen fit by any open source project, Linux distribution or anyone shipping Mycroft Core like Plasma Bigscreen, Plasma Desktop, Plasma Mobile, Open Voice OS, etc. If one is asking why put something like "Platform" in a configuration file if it is not configurable then I would like to point to https://github.com/MycroftAI/mycroft-core/blob/dev/mycroft/configuration/mycroft.conf#L233 which clearly is defined in a Mycroft configuration and also as a SYSTEM override while clearly implies that any Platform, Linux distribution or Open source project should be able to freely to set their own Platform or any other default configuration value for their project that comes under the "SYSTEM" override that does not get affected without actual user taking action and is only ever overridden by "USER" override. If the project does not want any other open source projects to use the configuration file to set their own system level values for system overridable things like "platform" and think it is the wrong place then what is the alternative solution that the project is suggesting that other open source projects should follow ? |
|
DEFAULT and REMOTE are defaults from Mycroft A.I. DEFAULT being the SYSTEM from Mycroft, REMOTE being the USER from Mycroft. If any other FOSS project want to do things different, above methodology can be shifted to utilizing SYSTEM being the system setting, USER being the users override of SYSTEM Mycroft != Selene and vica versa. Switching 2 and 3 changes the original methodology and screws up any other project that want to do their own thing. If you want to switch 2 and 3 you should create a two way sync, making sure USER and REMOTE play nicely together, basically making selene a webbased version of mycroft-config command for the USER settings it can configure and the other way around. For a quick fix for the mark2 dev kits, move all your /etc/mycroft/mycroft.conf and ~/.mycroft/mycroft.conf option over to the DEFAULT config of mycroft and empty both of those config files. (But expect bug reports if people use the mycroft-config cli commands and/or use voice commands such as: set listener to .....) But the real fix is tomake selene the webbased version of the mycroft-config CLI command to alter the USER config. just not flip 2 and 3 as the quickfix |
|
I think what Chris was heading toward with the point about As J1nx kind of suggested, we can also monkey patch the DEFAULT config for the Mark II in the short term. I wouldn't want to change the actual DEFAULT just for the Mark II. I'm still a little confused as to how the REMOTE config being moved down to 3 will mess with anyone's setups whether that's an individual or a project. If it does, then we can't sync REMOTE and USER or that would presumably also cause the same breakage? Can you give an example of when or how this would cause an issue? Two-way sync is certainly the end goal but will take a lot more work to do that. This feels like a step in that direction though. If there are going to be issues having REMOTE (or USER synced with REMOTE) configs override SYSTEM then we need to understand what they are. |
|
Ok, I wanted to come back and summarize where I think this is at - number 5 being the sticking point.
The only configs that I've thought might be an issue is for the wake word and TTS settings. However the REMOTE config doesn't provide a definition for "hey mycroft", only that it should be used. Likewise for Mimic2 it gives a "voice" attribute but no config for say url if a project was hosting their own mimic2 server. So someone using OVOS and selecting "Hey Mycroft" in Selene would get the OVOS configuration of that hotword. That would differ from the config someone would get on the Mark II OS and selecting the same button on the backend. If the REMOTE config got moved down as proposed, we would need to ensure any future configs in REMOTE didn't override existing configurations in a way that isn't 100% clear and expected by the user, eg changing wake words as above. If I'm missing something please point me in the right direction. Currently I can't see anything breaking by merging this PR. I also don't think this is just a quick fix, it aligns the two user defined configs that will happen when we have two-way sync anyway. |
|
[This post has been edited a bunch of times to expand on the initial ideas] the the problem arises when the If we embrace the concept that
|
I overall agree with the 4 points mentioned here.
Defaults can be any key/value set by a project in the future, for example Plasma Bigscreen uses a USB remote microphone and might want to ship different defaults in the SYSTEM config: This could be any of the ones mentioned here in the remote configuration #2861 (comment) that any project should have full control over at the time of shipping an image. Unless these values are then changed by the user manually in Selene or any backend while pairing, or by logging in and manually changing them for a single device, then and only then the changes logically should get written into REMOTE/USER configuration that overrides SYSTEM configuration, because these are REMOTE/USER changes that the USER is then aware making for any selections and changes. I as an external project maintainer would hate to later have to file a bug with this issue where I cannot ship configurations out of the box that are optimized for that project, which then largely as a bug report is going to be put on the backlog because it might have no priority above the mark-2. So for me at least how i see the actual solution:
For the above the first actual thing that needs to be working is the two way sync. |
|
I would like to add that Selene is not the only backend available at the moment and as it is a FOSS project, anyone could develop and/or use their own backend. So it is a bit short sighted to just focus on the current Selene backend parameters. It is more about the "attitude" of willing to change underlying architecture just to fix a Mark-2 "bug" Anyhow: Good to see that you reach out to the dev community in this way, before changing these type of things. |
|
small note, the 2 way sync isn't really needed in the options i mentioned in my previous comment, it's more of a usability thing than anything else it should be possible to send only the keys that actually changed without 2-way-sync the advantage of 2-way-sync is that the scheme I proposed in OPTION 2.5 removes this need, since |
|
Lots of interesting options here... currently Jarbas's option 2.5 is my personally preferred option. This means that a project could protect any and all attributes if they really wanted to. The listener sample_rate and channels are a very good point, though I think this is also something we'll need to address in the audio stack as I've seen recently that the default sample_rate at least is hardcoded in a number of locations. So changing that value is problematic all around. Also if they're specific to a hotword config, they should be in that specific hotword configuration, not the overall listener config. If they're really for the listener then this should either be software or hardware dependent, hence don't make sense as a user configurable config in REMOTE. Even if it stayed in the Selene REMOTE config it could be protected with an allow/block list of attributes in the SYSTEM config as suggested. I do want to push back a little that I'm just thinking about Selene or the Mark II. It's come up because of seeing it in the Mark II but when I looked at the current order it didn't really make sense to me. At least when thinking about dedicated devices and it seems that the current OVOS config shares the same issue for TTS engine. My first assumption was that it was designed for multi-user desktop environments. Thinking of it more as an override than a preference order makes more sense. Either way I think one of the options outlined in this thread is a better way forward. This is also why I wanted to flag this change specifically because it's way too easy to make assumptions that other projects are using something (and thinking about it) in the same way you yourself are. |
|
It seems like the main sticking point here is SYSTEM level configs that should never be changed by REMOTE (or USER) level configs. This returns me to my previous point. Why have something in a configuration file that should not be configurable? Forget for a second how the config system currently works. How SHOULD it work? If we can agree on that, then we can determine a plan to get there. There are two different concepts here that need to be addressed.
IMO, combining these two concepts into a single implementation is not a good idea. The business rules behind them are different. As such, they should be treated as separate entities. Configuration files:
Settings files:
Maybe "settings" is not the right term for what I am proposing they do but I hope you get the idea. @j1nx This is not just an issue for the Mark II. It is currently an issue for the Picroft as well and could potentially be an issue for any platform. |
|
you are mixing up configuration with permissions these things are meant to be configurable, they are not hardcoded settings, the thing is that they need i think #2872 addresses all the concerns raised here |
|
@chrisveilleux Yeah, again bad word choice of me. Indeed OVOS and Picroft run into the same issue. What I meant was, to quick fix the couple of Mark-2 bug reports as linked within this PR. Anyhow, doesn't matter as it looks like we are all at the same page here. |
Useful for removing specific attributes from a deeply nested json object such as a mycroft.conf file
|
i would like to keep remote and user seperated there is no official way to disable the backend, so privacy conscious users might want to block remote only, for example, if i do not want to disclose my location i can input garbage data in selene and not have it synced down at all. same goes for all other preferences i think there is value in allowing selective blocking, but at very least there should be a global remote disable flag, privacy first right? EDIT: the only change i would make in #2872 would be the usage of |
|
class DummyConf(RemoteConf):
def __init__(self, *arg, **kwarg):
pass
mock_remote.return_value = DummyConf()EDIT: I take it all back, much harder to fool isinstance than I thought Simplest way to do it seems to be to instead of mocking the RemoteConfig mocking the RemoteConfig.new, which seems to work...See here My suggestion for testing the pruning of the remote config is to extract the code into a function with input arguments and returns which can be tested without needing to run the entire config creation function. def prune_config(target, prune_config):
[...]Then call it via something like prunded_remote = prune_config(remote_config, system_config['system'])Then testing of |
Also fixes test failure caused by isinstance check
|
Voight Kampff Integration Test Failed (Results). |
|
@forslund thanks for that! @JarbasAI - I'm pretty hesitant to provide distinct behaviour for the two now as it seems they are likely to become one and the same in the future. It also seems like every project would just have two lists of the same protected keys for both remote and user.
You can still put garbage data in REMOTE and USER config will take ultimately priority.
It seems like people concerned about location or device settings from REMOTE would also be concerned about having any backend at all, particularly for STT and TTS. Instead of building work arounds, I think we add in an official way to disable the backend. |
|
Voight Kampff Integration Test Failed (Results). |
krisgesling
force-pushed
the
feature/re-order-configs
branch
from
f132dba
to
9264749
Compare
|
Voight Kampff Integration Test Failed (Results). |
|
here is an example, in OVOS i want to allow the user to change platform if he needs to, maybe it's a dev and want to pretend its a mk2 in purpose, on the other hand i explicitly want to block selene from sending that value when it adds some fancy new dropdown to the pairing process those two configs are not the same, one is controlled by you, the other is controlled by a user with physical access, i might want to stop you from imposing system values like the listener_rate, but if a user added some special hat to the raspberry pi i don't want to stop that use i can think of many more cases where i want to block disabling the backend is a great PR also, but in the end not related to this, i want you to stop you setting values like |
|
remote settings are really user settings. the user sets them, after all. Selene does not force any settings that are not defined by the user, nor will it ever. they will probably be combined in the future with some sort of synchronization, similar to skill settings. also, +1 to a system level config indicating remote configs should be ignored. |
mycroft/configuration/config.py
Outdated
| def prune_config(config, prune_list): | ||
| for key in prune_list: | ||
| config = delete_key_from_dict(key, config) | ||
| return config |
There was a problem hiding this comment.
Should this be a static method? I think this is mainly an internal thing and doesn't really need to be shipped with the Configuration object to the external caller. To me it can be a module level function. (also docstring)
There was a problem hiding this comment.
good call on both fronts :)
|
Voight Kampff Integration Test Failed (Results). |
krisgesling
force-pushed
the
feature/re-order-configs
branch
from
0843a5f
to
9fc3635
Compare
krisgesling
force-pushed
the
feature/re-order-configs
branch
from
9fc3635
to
555ec6c
Compare
|
Voight Kampff Integration Test Failed (Results). |
Also shifts config params to top level
krisgesling
force-pushed
the
feature/re-order-configs
branch
from
555ec6c
to
e26196e
Compare
| Dict: original dictionary with specified keys deleted. | ||
| """ | ||
| modified_dict = copy(dictionary) | ||
| key_list = key.split('.') |
There was a problem hiding this comment.
should the separator be made into an optional argument ?
it's possible dict keys will contain the default one, so this should be configurable
There was a problem hiding this comment.
This is true, but then we need a way to configure that delimiter so we'd be adding another mycroft.conf attribute.
Should we just use the right data structure to begin with and make it a list of lists of strings...
[['nested', 'key', 'example'], ['second', 'key']]This seems really painful to write out if you want to actually use it but who knows how often it will actually get used and only has to be done once
There was a problem hiding this comment.
|
Closing PR since we're archiving the repo |
Description
This re-orders the
mycroft.confpriority order to use remote settings over device defaults.Currently the priority order is:
/etc/mycroft/mycroft.conf~/.mycroft/mycroftThis switches 2 and 3 so that devices can declare default values that differ from the default mycroft-core but can also be overridden by the user modifying settings on home.mycroft.ai or their preferred backend.
How to test
Change the voice on a Mark II Dev Kit - this will not change because the device level settings have Mimic2 as the default voice.
Contributor license agreement signed?