Refactor brain class loading

[bjuergens]

implements #54

[bjuergens]

basically this is the same as #41, except with brains.

Warning: when this is merged, there will be a lot of conflicts with #49

PR can be directly merged, if review ok.

Note to reviewers:

• I am not 100% if this is actually an improvment to before. The main problem before this PR was, that experiment.py needs to be modified everytime a brain was added. With this PR changes (and especially additions) to the brains, will not require any changes outside the brain class and it's respective config. I think the thing that was violated before this PR is "open closed principle", but I am not 100% sure.
• This is a short review, but I think it is important that you know about this changes, since it affects the core of this program

[bjuergens]

Ahh, i think I understand. The idea of re-using the registration from the config didn't even occurred to me. That's a good point. I will think about it some more.

[bjuergens]

ok. I have implemented the idea. The bulk of this implementation is in helper.brain_class_from_config_type. It just gets a list of all classes, that are derived from IBrain. And then it selects the subclass that uses the expected config-class.

And I have found a bug with the old implementation (which also affects the new one, I think?). When brain-class are never explicitly imported, the code in their respective files is never executed. In the old implementation this means, that the register-decorator is never executed. In the new implementation this means, that the sub-class is never created. I solved both cases by using brains.__init__.py to import all brains in the folder.

Also for the new solution to work, the generics that links the braincfg-class to the brain class must be clean. I have solved this for the existing brains.

there are many commits listed here, because I have merged the master.

I am not sure how harmful the added complexity here will be. There is a lot of tomfoolery with python's module loading and class-parsing going on. I feel like it could make problems later on. But I can't think of any possible problems.

@pdeubel what do you think: Is this better or was it better before?

[DanielZim]

I generally like the idea that the experiment.py does not need to be modified whenever a new brain is added.

As far as I understand the code, in the new helper.py file the IBrain class is (recursively) iterated over its subclasses to add them into the all_brain_classes list. I guess, a potential issue with this concept can be, that unintentionally there could be subclasses added to this list that shouln't, since there might be some subclasses of IBrain (maybe added later on) that you are not aware of.

I think a "natural" approach to register classes to a list are class decorators (using something like @register('CTRNN') to explicitly add this class). At least the projects that I have seen use some kind of decorators to handle this, since you have a better control over which classes are added to the list.

[bjuergens]

I think a "natural" approach to register classes to a list are class decorators

Actually that way it was implemented in the first version of this.

But we noticed, that each brain has to be registered twice: on time for the config-class, and then again for loading the brain-class. I was looking around for ways to get around this double-registering, as it is on it's own a good source of complexity and errors.

Actually I have an idea...

[bjuergens]

ok, i have implented my idea here: https://github.com/neuroevolution-ai/NeuroEvolution-CTRNN_new/compare/ref_brain_class_loading...ref_brain_class_loading_idea1 (The implementation needs to move the cfg-class into the file where the brain-class is defined, because otherwise there will be circular imports with the config-reader)

but while implementing it, i had a better idea: I can leave the configs where they are, and don't register them directly. And use a decorator for the brain-class to register both the brain and the config

[bjuergens]

ok, i have now implemented a better idea.

notable changes;

• configs-classes for brains are not explicitly registered with @register like other config classes are
• brains-classes are now registered
• when a brain-class gets registered, introspection is used to find the config-class which is bound to this class. Then this the @register is called for this config

[DanielZim]

Thank you, I really like the idea to only register the brain classes, not the config-classes. I think this is cleaner and also better understandable.

Concerning the introspection in the register-function ("introspection into the class, its base class and its generics to find the config-class bound to this brain"), I wonder if this complexity is really necessary. I am just thinking if a (quite easy) solution like this would work:

The config-classes are not defined in the tools/configuration.py but as local classes in the corresponding classes that use it. For instance, the ContinuousTimeRNNCfg-class is defined as a local class in the ContinuousTimeRNN-class (in continuous_time_rnn.py). Then, the init function of the ContinuousTimeRNN-class receives the parameter "config" as a dictionary and creates the ContinuousTimeRNNCfg-object from it at the beginning of this function. I guess, the ContinuousTimeRNN-class is the only one that uses the ContinuousTimeRNNCfg-class and therefore, using a local class makes sense in my opinion. I think the same is true for the other classes that use their corresponding config-classes.

This proposed implementation is quite simple and maybe I am missing something here. Another advantage is, that we don't need to handle generics for the brain-classes and moreover, the config-classes are directly defined in their corresponding classes that are using them and not separated in tools/configurations.py (holding stuff that belongs together at the same place). We also would not need the replacing-dicts-with-types-function and the registration of the config classes which makes a couple of thinks easier I guess. On the other side, the attr-classes for the configuration are still used so we would not lose strict checks.

What do you think? As I wrote above, maybe I am missing something here.

[bjuergens]

I have opened a question in SO about this: https://stackoverflow.com/questions/66098618/how-to-implement-a-function-in-the-base-class-which-returns-the-type-of-a-generi

The config-classes are not defined in the tools/configuration.py but as local classes in the corresponding classes that use it.

Actually I have implemented something similar: https://github.com/neuroevolution-ai/NeuroEvolution-CTRNN_new/compare/ref_brain_class_loading...ref_brain_class_loading_idea1

But I like the idea of having all config-class in one place, so a user, that is writing the json-file, knows which paramters are available and/or expected.

I guess, the ContinuousTimeRNN-class is the only one that uses the ContinuousTimeRNNCfg-class

actually some of the newer brains share config-classes between them, but not important. If needed, it's simple to have them use their own config-classes again.

This proposed implementation is quite simple and maybe I am missing something here

No, I think your idea would work quite well. But it would require a lot of code to be moved around, so I would like to explore all options before making a decision.

What do you think? As I wrote above, maybe I am missing something here.

I like the locally principle. Each config-class is actually inside the class, that uses it. That would be great for readability and code-complexity.

But I also like the idea, of having all config-types in on place, so when writing a json-file, you always know which parameters are available. I think we could get around this. Because we already register each config-class (so that the json-reader can find it), we could add a CLI-parameter, which simply prints all found config-classes (similarly to how TAP does it). Then we have the best of both ideas: We have local config objects on one hand, and a global view on all available configurations.

a possible problem with local config classes: Maybe the parsing of the json-file becomes more difficult, because the parser has trouble to find the local classes?

[bjuergens]

the init function of the ContinuousTimeRNN-class receives the parameter "config" as a dictionary

I don't like the idea of passing dictionaries to the classes, but I don't yet know why I don't like it. I need to think about it some more... I guess I feel like dicts are unclean, and are a error-source in all code they touch. So I want to get rid of them as soon as possible. At the moment the config-dicts don't even enter the Experiment-class (they are converted to config-objects by train.py before passing them to the experiment) and I like that very much.

In anycase we would need to register the brain class (, or the config, or both), so the experiment-class (or whoever initiate the object from the json file) knows which constructor to call for the specific dictionary. So in both cases, there is already one place in the code, which both has the config-dict and knows which config-class to create from it. If we would pass the config-dict to the classes, that we would have to implement the dict-to-config again in all classes that use a config (brains, eprunner, optimizer, etc)

[bjuergens]

the more I think about local config-classes, the more I like the idea.

On a side-note: Maybe the config-class don't need to be inside the main class, and just in the same file next to it. I feel like this would make it easier for the config-loader to find it, maybe

[bjuergens]

ok, i have implemented yet another option: The config-class is passed as a parameter to the register_brain decorator. This adds a littlebiit of DRY-violation, because the same config class needs to be passed to the decorate and the type-hint. But on the other hand this is much cleaner, and the type-hint was only a optional nice-to-have to begin with. (even though I'd love to have more strict typing in this project)

I feel like this solution is fine.

And it's probably compatible with anything we may want to do in the future (e.g. moving the config-class-definitions closer to the class, that will use them, as discussed in the last 3 comments).

I think this PR should be merged as it is. And for the discussion about moving the config-class into the same file as the class that uses it, we should make a new PR

[DanielZim]

Yes, I agree on the side note, having the config-class just in the same file should be fine.

I think passing the dict as arguments is really the key idea of my proposed approach, since it would make the following things unnecessary:

• The _replace_dicts_with_types-function in the tools/config_reader.py
• The register-function in the tools/configurations.py and adding decorators to a couple of classes in this file
• The introspection functionality in i_brain.py

I think, these mentioned points add quite some complexity and at least for me it took some time to understand these mechanics reasonably. I agree, that it feels cleaner to fully convert the configuration dictionary to classes, but a) this unfortunately comes at the cost adding the shown complexity and b) after thinking a while about it, in my opinion we don't lose any clean, strict checking of the dictionary by passing dicts. In this solution, the checking is not done at the beginning, but it is done as well in each class (that uses a config class) directly in the respective init function. So we would leave the initial dictionary (after parsing it from the json file) as it is and pass this dictionary to the experiment class, which immediately does the strict check using the attr config class. Then, the "sub"-dictionaries like brain, episode_runner, ect are passed as dicts and their validity gets immediately checked at the beginning of the respective init functions. If there are sub-sub-dictionaries, this propagation does work as well.

So in summary, each class using a config-class strictly checks in their init function the received dict through their corresponding attr-config-class and I guess, we get the same clean, strict checking of the configuration as before while reducing the complexity, since we do not need the functions listed in the bullet points above.

This is just a further explanation of my point of view, what do you think?

[bjuergens]

I disagree completely. I don't think it would reduce complexity. And I think it would create a lot of inconsistency across all parts of the code-base in the long-run.

it would make the following things unnecessary:

• The register-function

Something like the register-function will always be needed. Otherwise there is no way to know which constructor must be used for a specific block in the json file. Even before we had the register-decorator, we still had a mapping from type-codes (e.g. "CTRNN" or "CMA-ES") to classes.

• The introspection functionality in i_brain.py

This is already gone in the current version.

• The _replace_dicts_with_types-function

This would not be gone, and instead it would be moved to many different places (i.e. each place, that converts a dict to a config-object).

Instead of doing this once (in _replace_dicts_with_types, called from train.py) we would do this in:

• CNN_CTRNN.py
• continuous_time_rnn.py
• elman,py
• ffnn.py
• gru.py
• i_layer_based_brain.py
• lstm.py
• episode_runner.py
• experiment.py

And worst of all: This opens us up for a lot of inconsistencies.

With the current way, we can always be 100% sure that each config block is parsed exactly the same way. When each class does the conversion of the dict on its own, then differences will eventually come. At the very least at some point someone will get lazy and say "fuckit, lets just use the dict an be done with it". Or someone will say "well... if the config is not readonly, then I could implement a specific feature quick and dirty".

By forcing the central parsing of the config, we ensure a lot of consistency across all parts of the code-base. If we didn't have the enforced consistency, every time you look at a new brain-class, you have to ask yourself "hm... but is the config really readonly as it is with the other brains? Are the config's values the same in the step-method as they were in the constructor? Is the config really parsed in the same way as with the other brains?". When there is a subtle difference in the parsing of the config (e.g. attr.s wasn't used), than you will need to put effort in to find out if this subtle difference is intentional, or a mistake.

with the current implementation of central config-reading we enforce, that the same design-patterns are used across the entire code-base. When we let go of it, then it will be only be a matter of time until some parts of the code base will diverge from other parts.

[bjuergens]

regarding the issue of moving the config-classes out of experiment.py I have created another issue: #72

regarding the issue of changing the way config-classes are instantiated, we should create a separate issue for discussion, as this specific PR is about discussion of the brain-registering and not about configs in general. I think this discussion is important though. And maybe I am wrong. Also I think, I will have to read up more on software-design principles, because I noticed that I have trouble putting my ideas into words. At first glance I think the my point of view is violating KISS, while daniel's point of view is violating GRASP. But as I said: I need to educate myself more on the issue, and we should move the discussion into a separate issue.

@pdeubel what do you think about this PR? (not about the discussion about config-class creation)