Post init option for class generator

[mavaylon1]

Motivation

What was the reasoning behind this change? Please explain the changes briefly.
Fix #1072

Add optional post init method for get_class (class generation for extensions)

How to test the behavior?

Show how to reproduce the new behavior (can be a bug fix or a new feature)

Checklist

• Did you update CHANGELOG.md with your changes?
• Does the PR clearly describe the problem and the solution?
• Have you reviewed our Contributing Guide?
• Does the PR use "Fix #XXX" notation to tell GitHub to close the relevant issue numbered XXX when the PR is merged?

[mavaylon1]

Note: This is not ready for review. I need to make tests. However, I think the concept is there to talk about. @rly @oruebel

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.71%. Comparing base (126bdb1) to head (898832b).
Report is 36 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1089      +/-   ##
==========================================
+ Coverage   88.70%   88.71%   +0.01%     
==========================================
  Files          45       45              
  Lines        9745     9760      +15     
  Branches     2767     2771       +4     
==========================================
+ Hits         8644     8659      +15     
  Misses        779      779              
  Partials      322      322              

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

[oruebel]

I think the concept is there to talk about.

The concept looks good to me

[rly]

The approach looks good to me too

[rly]

Note that you will have to be careful with methods vs functions here. It seems like the post_init used in ExtenderMeta acts on the class and not the instance, and most likely users will want to run code on the instance. You may need to create a separate post_init system for generated classes that allows you to do that.

[mavaylon1]

@rly I still need to add the post init to the MCIClassGenerator, but I have a new proof of concept and a test that works.

[mavaylon1]

@rly I was wrapping this up with test on the MCIClassGenerator, adding post_init to the init method defined in set_init. What's weird is my test test_multi_container_post_init uses MCIClassGenerator (I verified by adding a breakpoint in the set_init for MCIClassGenerator), but the init that is called is the CustomClassGenerator (Verified by both the lack of coverage and adding a breakpoint in the init).

I see that MCIClassGenerator inherits from CustomClassGenerator, but the set_init function in MCIClassGenerator should override. It does (see above) but not in a way I expect. Thoughts?

[rly]

In https://github.com/hdmf-dev/hdmf/blob/dev/src/hdmf/build/classgenerator.py old line 334, the __init__ generated by CustomClassGenerator is called on a subset of kwargs, which calls post_init . After that is done, the outer MCIClassGenerator calls post_init again. Does that answer your question?

[mavaylon1]

https://github.com/hdmf-dev/hdmf/blob/dev/src/hdmf/build/classgenerator.py

I actually think its from old line 440. With MCIGenerator caling the init within CustomGenerator, do we only want to run the post init for MCIGenerator after the outer init (the init in MCI) and not the inner init?

[rly]

You're right - I was looking at the wrong class there. I would say after the outer init.

[mavaylon1]

@rly take a look when you can. I need figure out a way to solve the duplication problem in docval. I believe what is happening is that if we generate class A and class B (which inherits from A), it uses the same docval in the set_init method. This duplicates the post_init_bool because *docval_args is the docval from the parent class. Thoughts on the overall approach?

[rly]

I see. The approach feels a bit hacky but I cannot think of a better approach. One idea is that when getting the parent class' docval args, you can remove post_init_bool from the result.

[mavaylon1]

I see. The approach feels a bit hacky but I cannot think of a better approach. One idea is that when getting the parent class' docval args, you can remove post_init_bool from the result.

Yeah that makes sense. I'm going to push this idea forward and make update tests.

[mavaylon1]

I will shoot to wrap this up tomorrow since it is very close and we have an agreed idea@rly

[mavaylon1]

@rly This should be good to go. I am going to do another pass before I request review. However, I wanted to note that the tutorial on this functionality makes more sense to me to be in the extension template, rather than here. What do you think?

[rly]

Let's document this in the NWB extensions tutorial rather than here https://nwb-overview.readthedocs.io/en/latest/extensions_tutorial/extensions_tutorial_home.html. We could do it in the template, but I would consider this more of an advanced feature.

[mavaylon1]

1. The user would call get_class. This method now has a new input parameter called post_init_method. It is a Callable and it is a function that will be used as a post_init method to validate something during the class generation. It could also be more broad in its functionality.
2. get_class calls get_dt_container_cls from the the TypeMap. This calls generate_class within the class_generator. We add the post_init_method as an input parameter here as well.
3. Within generate_class we do two things of interest. The first is we set the init method in the class. This is done from a class generator. This will be in either CustomClassGenerator or MCIClassGenerator. We simply call the post_init method at the end of the init method being set using the original kwargs. The second thing we do is set the post_init_method to the class via cls.post_init_method = post_init_method.

Let's talk about some details:

1. We can have classes that are generated where the parent class is also generated with a docval field that already has skip_post_init as an arg. We remove it before setting the docval of the new init. (I can't think of another way. It's not a bug, but just a product of generating classes)
2. Within MCIClassGenerator, we call previous_init. This means the init generated by CustomClassGenerator is called on a subset of kwargs. We want to make sure that the post_init is not called here, but rather at the end of the init set within MCIClassGenerator. That's why we have a skip_post_init.

[rly]

Looks good to me. Thanks!

[mavaylon1]

@h-mayorquin
Hey Heberto this is the post_init method you were requesting. To use it, simply add a method into the post_init_method parameter in get_class (without the the parenthesis).

[h-mayorquin]

Thanks, yes, I saw that you added, I will give it a test in the following weeks : )