chg: Use pydantic to manage config schemas.

[unixtreme]

#69 this will conflict with #109 if that merges earlier I have to update the temporary hard-coded modes.

As part of the final PR I plan to:

• Improve how data can be accessed, allow to access data as if it was an instance attribute. e.g. global_config.obs not global_config['obs']. This can also be done dynamically very easily and I'm quite familiar.
• Profiles need to load and update their own config if present.
• catch_block is reloaded on every shiny encounter
• Add support for saving the data stored back into the original file.
• Add some sort of UI element to reload config, I am not a fan of reloading automatically since we may reload while a user is still tuning the settings.
• Add CLI overrides.
• Add unit tests for config load.

[hanzi]

A couple of observations -- and sorry if that sounds all too negative, just concentrating on the things that might be problematic 🙂

I feel like this makes config loading more complicated by introducing a 3-level class inheritance structure, without actually changing very much how it works.

While this does introduce classes, these don't really seem to offer much benefit over the old '3 functions that load everything' implementation: This still doesn't offer any auto completion when editing code (because all values are loaded dynamically), and because the structure is still defined in a JSON schema string it doesn't make it any easier to eventually write a config editor.

Also, if I understand the code correctly, this removes the option of overriding individual values in profiles. That would probably make a few people rather unhappy who currently rely on configuring different starters, Discord webhooks, etc. per-profile.

The code seems to imply that there are some settings that have to be configured globally, and other settings that have to be configured on a profile level. (If I'm misinterpreting that, sorry and please disregard this comment.) This doesn't currently hold true: Every setting can either be configured globally (in the profiles/*.yml files) or in a profile-specific file (profiles/<profile name>/*.yml) where the latter would take precedence.

[unixtreme]

Thanks for the feedback, I'm very new to the project and I have limited time available so there are quite a few things that I'm not familiar with yet so it's very helpful.

I feel like this makes config loading more complicated by introducing a 3-level class inheritance structure, without actually changing very much how it works.

I think I misinterpreted a few things about the original intent. My intent here was just to setup a framework but since it was a big departure I didn't want to actually work on any of the actual improvements in case it wasn't welcome (and I'm glad I held off :P).

While this does introduce classes, these don't really seem to offer much benefit over the old '3 functions that load everything' implementation: This still doesn't offer any auto completion when editing code (because all values are loaded dynamically), and because the structure is still defined in a JSON schema string it doesn't make it any easier to eventually write a config editor.

I was under the wrong impression that we wanted the original schemas, personally, I prefer pydantic schemas, in which case we'll have IDE integration.

Also, if I understand the code correctly, this removes the option of overriding individual values in profiles. That would probably make a few people rather unhappy who currently rely on configuring different starters, Discord webhooks, etc. per-profile.

The code seems to imply that there are some settings that have to be configured globally, and other settings that have to be configured on a profile level. (If I'm misinterpreting that, sorry and please disregard this comment.) This doesn't currently hold true: Every setting can either be configured globally (in the profiles/*.yml files) or in a profile-specific file (profiles/<profile name>/*.yml) where the latter would take precedence.

That was a big miss on my end, I was going to implement that when I got to Profiles, and have them update the defaults or use their own settings if they have been set. But I would need to do everything at once. I'll go back to the drawing board and try to make a simpler, just in case a couple of questions:

• It feels like pydantic does exactly what we would want, as opposed to yaml strings. Does that sound right?
• Since settings can be updated by profiles, it feels like just updating the context config with whatever settings have changed by the profile config when a profile is loaded should take care of that.
• Any consideration for the ability to load multiple profiles in the future?

Part of what made me approach this in such a different way was that in the back of my mind I had the idea of eventually adding a setting that would detect the CPU capabilities and allow fanning profiles in multiple processes orchestrated by the global context. But if there's no need for anything like that and the plan is to keep the bot strictly as a single-emulator single-profile approach there's no need to future-proof anything.

[unixtreme]

Converted to draft showing the general idea of managing the config schemas with pydantic schemas.
I chose confz because it's a light wrapper and allows specifying multiple sources for the config, so adding CLI overrides later on should be easy.

This is just a POC of what I could cobble together in a couple of hours to gather feedback before I invest further time, as touching the config means touching basically everything.

I still have to override config with config from profiles but that shouldn't take too much work. And I have to rebase since this PR looks like I'm deleting random stuff from newer merges, lol.

[hanzi]

This looks quite nice! pydantic seems like a good fit for that.

I have a couple more thoughts on this, but first to preface here are the issues that I'm seeing with the current config system:

1. The config files in the profiles/ folder serve both as a set of default values, as well as the global config. This means that we are pushing users to edit files that are controlled by Git (or, if they just downloaded the ZIP, might get overwritten each time they update the bot.)
2. While we support per-profile configuration, this requires the entire config file to be copied and modified instead of allowing to specify individual config options. That makes adding configuration values more annoying for everyone involved as the profile-specific config needs to be updated. (The same issue occurs when modifying the global config files for the reason specified in (1).)
3. People occasionally struggle with editing YAML files because while much more human-readable than other formats such as JSON, it still has some unintuitive (to the non-techy user) syntax constraints such as indentation levels, requirement to have a space after the : etc.

So my thinking is:

• Issue (2) is probably already solved by the config objects you've created using pydantic.

• Regarding issue (1), should we perhaps get rid of all the default profile/*.yml files in favour of defining default values in-code, and then we just have a config.yml or something like that with all config options commented out, so users can choose to override whichever values they like while leaving everything else untouched? This config.yml could even be auto-generated on first bot startup, so it doesn't need to be controlled by Git at all.

• Likewise, there could be a profiles/<profile name>/config.yml for each profile to have 'local' overrides same as now.

• To make migration easier, we could keep the old config loader around for a while and use it to load the legacy YAML files in case there is no config.yml. We could compare all values from those legacy YAMLs to the default values, and just write out all the non-default ones to that file.

• Thinking ahead a bit towards a potential config editor GUI, I think it would be easier to have a flat config structure rather than some nested objects. While I agree that the latter 'feels' a lot better and more correct from a developer perspective, having a flat key/value structure would make an auto-generated config GUI easier to do. It could be a window with a list of all config options, each consisting of a label and a datatype-appropriate widget (text entry field, slider, dropdown selection, ...)

• If we agree on a flat config, perhaps we could switch from YAML to INI files using Python's built-in configparser module. INI is a bit more forgiving for users to edit (and at least from what I understand, pydantic can work with those objects too.) For some simple structuring, INI supports sections, e.g.:

  [general]
  starter = Mudkip
  
  [cheats]
  ; configparser accepts 'yes', 'no', 'on' ,'off', 'true', 'false', '0', and '1' as boolean values
  ; see: https://docs.python.org/3/library/configparser.html#supported-datatypes
  starters = no
  starters_rng = no
  
  ; ...

• A flat structure might make it mildly more convenient to have overrides-by-CLI-args.

[unixtreme]

This looks quite nice! pydantic seems like a good fit for that.

I have a couple more thoughts on this, but first to preface here are the issues that I'm seeing with the current config system:

1. The config files in the `profiles/` folder serve both as a set of default values, as well as the global config. This means that we are pushing users to edit files that are controlled by Git (or, if they just downloaded the ZIP, might get overwritten each time they update the bot.)

2. While we support per-profile configuration, this requires the _entire_ config file to be copied and modified instead of allowing to specify individual config options. That makes adding configuration values more annoying for everyone involved as the profile-specific config needs to be updated. (The same issue occurs when modifying the global config files for the reason specified in (1).)

3. People occasionally struggle with editing YAML files because while much more human-readable than other formats such as JSON, it still has some unintuitive (to the non-techy user) syntax constraints such as indentation levels, requirement to have a space after the `:` etc.

So my thinking is:

* Issue (2) is probably already solved by the config objects you've created using pydantic.

* Regarding issue (1), should we perhaps get rid of all the default `profile/*.yml` files in favour of defining default values in-code, and then we just have a `config.yml` or something like that with all config options commented out, so users can choose to override whichever values they like while leaving everything else untouched? This `config.yml` could even be auto-generated on first bot startup, so it doesn't need to be controlled by Git at all.

* Likewise, there could be a `profiles/<profile name>/config.yml` for each profile to have 'local' overrides same as now.

* To make migration easier, we could keep the old config loader around for a while and use it to load the legacy YAML files in case there is no `config.yml`. We could compare all values from those legacy YAMLs to the default values, and just write out all the non-default ones to that file.

I agree, I kind wanted to add the current defaults into the schema, then only modified values will be loaded in, allowing for a config file with say a single setting someone cares about regardless of it being in a global file (for legacy support purposes) or in a particular profile, where maybe the user just wants like a single webhook configuration file or something. This would solve (1) and (2), I didn't jump the gun because I wasn't sure if it was preferred but it looks like an improvement to me.

Additionally, add the old config file definitions to .gitignore and remove them from the repo, ensuring there are no further modifications to user config if they merge a folder with a new version on top of their current installation.

* Thinking ahead a bit towards a potential config editor GUI, I think it would be easier to have a flat config structure rather than some nested objects. While I agree that the latter 'feels' a lot better and more correct from a developer perspective, having a flat key/value structure would make an auto-generated config GUI easier to do. It could be a window with a list of all config options, each consisting of a label and a datatype-appropriate widget (text entry field, slider, dropdown selection, ...)

* _If_ we agree on a flat config, perhaps we could switch from YAML to INI files using Python's built-in `configparser` module. INI is a bit more forgiving for users to edit (and at least from what I understand, pydantic can work with those objects too.) For some simple structuring, INI supports sections, e.g.:
* A flat structure might make it mildly more convenient to have overrides-by-CLI-args.

This also makes sense to me, when I was making the configuration trees I wasn't very happy about the user experience in general of using nested logic or YAML files for the same reasons you mention. I didn't want to come through as opinionated saying "we have to nuke all this" and it also seemed like it would expand the scope significantly.

Personally, the way I tend to approach these things is in approachable steps on different PRs, because it makes reverting easier and if something goes wrong, reduces friction with other developers that will start getting used to accessing information from the config in a slightly different way, and doing the most drastic change to moving to models as son as possible reduces the problems with merge conflicts in the future, since IDE refactor tools can be used instead of manually hunting down every access to config as a dict, so changing the shapes of those schemas becomes a lot easier once they have been implemented. So for example:

• Initial Change: Move config to a schema system keeping functional parity, add some QOL things a long the way like a reload on the GUI, get rid of static mandatory files with defaults in favor of defaults inside the schemas.
• Transitioning Change: Add schema versioning support (e.g. v1 would be YAML v2 INI based), keep both from a user perspective but convert the data internally so the program only really deals with the new version. Prompt the user with a warning that legacy YAML file support will be dropped at a later date and also save the INI equivalents. Add CLI overrides since I think you are right, it will be a lot more digestible for users with a flat config.
• Completion: At a much later date one can drop all the transitioning stuff and old schema models and only support INI files, if drastic config schema changes that break the old INI are required we can go back to another transitioning change, rinse and repeat.

If you prefer 1 and 2 to be done together I think that's doable, but in general breaking it up into manageable chunks could allow easier collaboration since someone may only have the time to do one half of a big change and someone else could come along and complete it.

[40Cakes]

Been busy, haven't had much time to properly read and catch up in this thread, but why .ini over .yml, just curious is there a benefit or requirement?

And also, don't stress about transitioning changes, people understand this thing is under heavy development and it's still an early version, if they need to nuke their config and start from scratch, so be it. I think it's easier to just rip off the bandaid, don't be afraid! 😄
I would rather not keep a legacy method around just to remove it later.

[hanzi]

@unixtreme Yeah I didn't mean to do all those things in this PR, just thought I'd mention them here rather than the ticket to keep the discussion in one place. 🙂

I agree, I kind wanted to add the current defaults into the schema, then only modified values will be loaded in, allowing for a config file with say a single setting someone cares about regardless of it being in a global file (for legacy support purposes) or in a particular profile, where maybe the user just wants like a single webhook configuration file or something. This would solve (1) and (2), I didn't jump the gun because I wasn't sure if it was preferred but it looks like an improvement to me.

Additionally, add the old config file definitions to .gitignore and remove them from the repo, ensuring there are no further modifications to user config if they merge a folder with a new version on top of their current installation.

I like that! So then, all existing config files would just continue working and there would be no breaking change? Nice.

---

@40Cakes

I would rather not keep a legacy method around just to remove it later.

I'm not sure I agree, at least in this case.

For starters, there is going to be a need for a migration algorithm either way -- whether that's an automatic migration coded into the bot, or an explanatory section in the Readme or an update post in some Discord channel.

I'd argue that it is easier to write and reliably test migration code than it is to write a good explanation on how to do that and then deal with all the follow-up confusion of people that are inevitably going to do it wrong anyway.

Secondly, the code for loading the old config format already exists, so all that would be needed is a couple lines of code that check whether old files exist, load them, and store them in the new format. And perhaps a bit of mapping code.

Lastly, I myself have about 20 profiles for various testing purposes. It'd be very nice if I didn't have to fix the config files everywhere. So even just for us some automatic conversion would be nice.

---

@40Cakes

but why .ini over .yml, just curious is there a benefit or requirement?

It's not so much about the file format itself and more about the structure of config values.

If we had a simple key/value table of config options it would be easier to auto-generate a GUI for that. Because you'd just have to for loop over that list of config options and for each one generate a label and an appropriate widget (text entry field, checkbox, dropdown list, ...)

At the moment, we have a config tree rather than a table because some values are nested (mostly in discord.yml, keys.yml, logging.yml and obs.yml.) 'Flattening' those nested values (for example, instead of config["obs_websocket"]["host"] you might just as well do config["obs_websocket_host"]) would allow for that easier GUI generation.

This could easily be done with YAML as well, but if we change the config files anyway we might as well consider the file format at the same time.

If we don't use YAML's nesting features, then INI would be a potential substitute that is more forgiving with its syntax. While I don't doubt that someone is going to manage messing up even that, the syntax rules are much more simple than for YAML (and also, I believe that on Windows *.ini is even mapped to Notepad by default.)

[unixtreme]

• I added all current defaults as default values in the schemas.
• Tested with and without files config changes in the files load fine.
• Added some save methods to save the current loaded config for whenever we add it to the UI.
• Added a button to reload config, I tested it and it works, with one exception, keys are not re-set if they have been changed in the config, I need to work on that.
• This button has been placed completely randomly just for testing no idea where it should be placed and I'm terrible at UI design:

This ruins the placement of the bot mode selection, although if we keep it there (with some size adjustment) I guess we could just align the Bot Mode box to the top.

[unixtreme]

Added some basic unit tests.

[hanzi]

There are a few other things that probably wouldn't get reloaded properly, such as Discord rich presence or the HTTP server. Which is not bad, just something to be aware of.

From a UX perspective, this button might be a bit confusing to users. Unlike all the other controls this one has no apparent effect when clicked, and the way it is positioned might be misunderstood as a 'save' button for the other controls.

While there is no harm in reloading the config if it has not changed, how about we drop the button for now and just hide that feature behind some keyboard shortcut such as Ctrl+C?

[unixtreme]

Oh Right I completely forgot those two run in their own process and are setup at the start. Yeah basically only values that are accessed directly are really doing anything.

Hiding it behind some shortcut instead of the UI button crossed my mind and it's probably better than cluttering the UI.

[unixtreme]

Rebased and solved conflicts, set the config reload to Ctrl-C and it now applies key config changes. I'll keep in mind the stuff that runs on a separate thread but I don't think that's super urgent since I don't expect those to be changes one really cares about doing while running.

[hanzi]

Code-wise this looks pretty good to me, though I haven't had a chance to actually test it yet.

Nice work! 🙂

[40Cakes]

Apologies for leaving this in the dark for a bit, been quite unmotivated lately...

Going to start reviewing and merge this before #109 and #117 as they both have config related code, should be fairly straightforward to rebase them anyway. Just going to post anything I find as I test.

  File "C:\Users\Ryan\Documents\GitHub\pokebot-gen3\pokebot.py", line 9, in <module>
    from modules import exceptions  # Import base module to ensure the custom exception hook is applied.
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    
ModuleNotFoundError: No module named 'confz'

Seems to break the requirements check, imported before check_requirements() in pokebot.py.

[unixtreme]

Apologies for leaving this in the dark for a bit, been quite unmotivated lately...

hey no worries we can't all have 100% energy all the time, this is a free-time side thing after all!

Going to start reviewing and merge this before #109 and #117 as they both have config related code, should be fairly straightforward to rebase them anyway. Just going to post anything I find as I test.

  File "C:\Users\Ryan\Documents\GitHub\pokebot-gen3\pokebot.py", line 9, in <module>
    from modules import exceptions  # Import base module to ensure the custom exception hook is applied.
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    
ModuleNotFoundError: No module named 'confz'

Seems to break the requirements check, imported before check_requirements() in pokebot.py.

Oops my bad, I did all testing with confz already installed and didnt try a new venv without any dependencies triggering a fresh install. I moved this import to after the requirements check.