Move logging config to separate yaml, configure individual loggers

[teutoburg]

When you discover you are riding a dead horse, the best strategy is to dismount.

TLDR

After thinking about this for a while and trying various things, I came to the conclusion that having the logging config in rc.__config__ (and thus in the bottom level of all the cmds nested chainmap thingies) is indeed such a deceased hoofed creature. Thus, to cut losses now and avoid any sunken cost fallacy, I decided to remove the logging stuff from there and put it into a separate place rc.__logging_config__, and a new, separate yaml.

But why?

Problems with logging in the chainmap

• The NestedMapping didn't fully support keys containing a . character in the string, because it created an ambiguity when using dot-separated "bang keys". See below why this can't be properly fixed.
• Having all the logging sub-dicts in the whole cmds can create quite a bit of spam in the various printouts containing cmds, while not really adding any useful information in virtually all cases.
• The idea of the chain map cmds is to be able to override configurations on a local level (e.g. optical train instance). This doesn't really work for logging, as the loggers as they are used in ScopeSim are global objects. Thus any changes made to logging-related keys in higher levels of the chain map will either have no effect at all or can potentially lead to conflicts between e.g. different optical train instances. Considering this, I think it's better, easier and cleaner to just treat logging as global. I can't really imagine many cases where you'd really need to configure it differently in different contexts at the same time.
• Somewhat related to the point above, the way the nested logging dicts are passed to the builtin configurator would be difficult (if not impossible) to achieve with the way the nested mapping classes return subdicts that are overridden on different levels.

Why do we even need dots in keys?

Loggers follow a hierarchical namespace structure equivalent to Python's package.module.whatever namespace, separated by dots. To be able to configure individual loggers by default in the dict config (which is the cleanest method of initially configuring logging), we need to be able to include keys containing dots in this nested dict.

Rejected alternative

I tried for a bit to enable keys containing dots for the NestedMapping class. In the end, I was able to make it work, but really only for retrieving existing keys. Adding a new key (or, to a lesser extent, modifying an existing one) created an ambiguity. Consider the following case: A NestedMapping instance contains a top-level key "XYZ", which leads to a sub-dict with the key "ab.cd" that in turn leads to a dict containing the key "m". Retrieving "!XYZ.ab.cd.m" can in theory (and indeed in practice, I did that) be implemented to look up the entire thing in a tree-like manner until the correct key is found. If a new item is to be set with the key "!XYZ.ab.cd.k", how is the NestedMapping supposed to know if that's meant to be a key "k" in the sub-dict "ab.cd", or in "cd", which is in turn a sub-dict of "ab" (and that itself of "XYZ")?

So, instead of implementing a hacky way to include these things and have potential ambiguities in there, I think it's preferable to simply accept it as a limitation of NestedMapping that keys cannot contain dots. As outlined above, this might have actually several benefits in the context of the logging config, in keeping it in a separate place.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.19%. Comparing base (ca93e40) to head (8a96af1).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #502      +/-   ##
==========================================
+ Coverage   77.17%   77.19%   +0.02%     
==========================================
  Files          66       66              
  Lines        8210     8209       -1     
==========================================
+ Hits         6336     6337       +1     
+ Misses       1874     1872       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[hugobuddel]

Seems this makes logging easier and less polluting, so good!

Another positive thing is that I think it is good to have everything that does not affect the simulation to be separate from things that do affect the simulation. E.g. one of my goals is that ScopeSim should be able to recreate the pixels from the headers, and the logging configuration is not relevant to that.

[hugobuddel]

Or to say the same thing in different words: the logging configuration is in a way configuration of an independent third party library, not a configuration of the simulator itself.

So there is no 'need' to put the logging config in the same file as the simulator config, and there are benefits to keeping it separate. E.g. now we could even use some kind of 'logging-native' configuration would such a thing exist (which I don't know). Or we can switch logging frameworks without affecting our core simulator configuration.

[teutoburg]

Another positive thing is that I think it is good to have everything that does not affect the simulation to be separate from things that do affect the simulation. E.g. one of my goals is that ScopeSim should be able to recreate the pixels from the headers, and the logging configuration is not relevant to that.

Agree. By this logic, we should also think about removing the "tests" sub-dict and its two flags from default.yaml. I never quite understood why those are there in the first place, I think that's something that should be handled within the tests dir itself, ideally using existing mechanics from pytest (maybe markers). But having those where they are now might lead to some unexpected results because someone might not expect them there. But that's a separate issue to this PR...

[teutoburg]

E.g. now we could even use some kind of 'logging-native' configuration would such a thing exist (which I don't know).

You mean like the builtin logging.basicConfig() (in horrible camel case 🐫)?

[hugobuddel]

Agree. By this logic, we should also think about removing the "tests" sub-dict and its two flags from default.yaml. I never quite understood why those are there in the first place, I think that's something that should be handled within the tests dir itself, ideally using existing mechanics from pytest (maybe markers). But having those where they are now might lead to some unexpected results because someone might not expect them there. But that's a separate issue to this PR...

Yeah, we can just use pytest for that. Never noticed that was there.

E.g. now we could even use some kind of 'logging-native' configuration would such a thing exist (which I don't know).

You mean like the builtin logging.basicConfig() (in horrible camel case 🐫)?

I didn't say we should actually switch the configuration, but to me it makes sense to be able to do so.

[teutoburg]

In the long run, once we separate scopesim-core (the library) from ScopeSim (the application), the current complex (but useful) logging config stuff should go into the application, whereas the library should provide logging, but not really configure much of it (with perhaps some exceptions). See also https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library.

That would enable any other applications (think exposure time simulator) to use the library in the backend, but add its own logging configuration, which might look very different from what we're doing in ScopeSim (especially when there's a GUI involved).

(Please ignore any potential exact duplicates of this comment, the Upper Friulian Subway has terrible connection and I don't know if and how often this is getting through 🙃)

[teutoburg]

E.g. now we could even use some kind of 'logging-native' configuration would such a thing exist (which I don't know).

You mean like the builtin logging.basicConfig() (in horrible camel case 🐫)?

I didn't say we should actually switch the configuration, but to me it makes sense to be able to do so.

Yeah, that's what I understood 👍