[Bf-committers] Auto-registration in Python

[Campbell Barton]

On Fri, Oct 29, 2010 at 10:06 PM, Nathan Vegdahl <cessen at cessen.com> wrote:
>> On the contrary. Having half-defined subclasses of RNATypes breaks
>> the Liskov substitution principle.
>
> How?  The RNATypes themselves are not fully defined (e.g. you can't
> use bpy.types.Panel directly).  Making a derived class of
> bpy.types.Panel that also can't be directly used hardly violates the
> Liskov substitution principle, as long as it can also be further
> derived from.  I'm not even sure how Liskov is relevant.
>
> It _is_ however a violation of the 'contract', as you stated later on.
>  So I see where you're coming from.  But still, these are rules of
> thumb, not things that should be strictly enforced.  Because sometimes
> it is useful to bend or break such principles, even if in the general
> case it is best to stick to them.
>
> My main complaint about the current system, however, is that it's so
> implicit.  It does things without the code telling it to.  And this is
> bad, both in terms of my personal preferences and in terms of code
> readability (i.e. being able to follow what's going on).  I would hate
> to use a GUI toolkit, for example, that automatically displayed
> defined GUI elements without them being explicitly linked somewhere in
> the main loop.  That's exactly what this feels like.
>
> The current system:
> 1. Coder defines a class.
> 2. That class magically shows up somewhere else without the coder telling it to.
> 3. wtf?
>
> A better system:
> 1. Coder defines a class.
> 2. Coder puts that class somewhere else.
> 3. No "wtf".
>
> Having said that, I'm coming to agree with you insofar as property
> types are concerned (e.g. something derived from IDPropertyGroup),
> since as a coder you expect to just be able to use the class directly
> in your code.  Having to 'register' it is strange.
>
> But things like operators, and especially panels(!) because they
> meaningfully change Blender's behavior, really feel like things that
> should be registered explicitly.
>
> --Nathan
>
>
> On Thu, Oct 28, 2010 at 9:26 PM, Martin Poirier <theeth at yahoo.com> wrote:
>> NOTE: TL/DR at bottom
>>
>> --- On Thu, 10/28/10, Nathan Vegdahl <cessen at cessen.com> wrote:
>>
>>> > It should be even more trivial
>>> with automatic registration.
>>>
>>> And yet in reality...
>>
>> Yet in really it's exactly as I was saying. Those problems concern properties, which aren't taken care off by the autoregistration system (yet), which really sucks.
>>
>> I agree.
>>
>> Part of the problem is that IDPropertyGroup derived classes, as a RNAType, is autoregistered while property definitions aren't, that's why property definition needs to be done in the Register function (especially in cases where they reference subclasses of IDPropertyGroup, which I would guess is more than a few times). [Well, that and otherwise addons wouldn't be good citizen and you'd end up with properties from all installed stuff, which is a huge NO).
>>
>> This isn't even a problem with autoregistration, the same issue would crop up with manual registration. You just can't define properties in the main namespace of a module if they reference IDPropertyGroup classes that will be registered later (whether automatically or manually in the Register function, as before). Also, the addons system won't work correctly in those cases, as mentioned before.
>>
>> One major problem is that there is (as far as I'm aware), no real written guidelines as to how addons should be coded in that regard.
>>
>> (In the future, as a solution, adding a delayed call system for properties would be the way to solve it, and that should pretty much be doable in the same way autoregistration currently works and pretty much remove the need for Register and Unregister functions, although we could still keep them around if people want to initialize custom resources).
>>
>>> Here's the thing: I'm speaking as someone who was not
>>> involved in
>>> designing the system.  For someone who doesn't have
>>> knowledge of the
>>> internals of how auto-registration works, it becomes a
>>> magical system
>>> with semi-unpredictable and unclear rules.  Like, what
>>> gets
>>> registered?  What doesn't?  How do I control that
>>> so that I can
>>> structure my code how I want to?  When do things get
>>> registered?  Etc.
>>
>> I agree, there's a serious lack of documentation.
>>
>> The rules are rather simple though:
>>
>> Anything that derives from an RNAType is automatically registered (and will work correctly with the addon system). Don't derive from an RNAType if your class isn't a fully formed RNAType (with all the defined methods and whatnots that this imply).
>>
>> Any properties that you want to add need to be added in a Register function.
>>
>> The ordering doesn't really matter. RNATypes will always be registered (in the order they are defined) before the Register function is called, so if you add properties to a IDPropertyGroup in your Register function, it will always work the first time.
>>
>> Where it fucks up is when turning stuff on and off in the addons system and registering a module (or a package) multiple time in the same blender session. That's a problem with the properties system, NOT with autoregistration.
>>
>>> And to make things concrete: it is not obvious, for
>>> example, how I
>>> should structure my code so that I don't get weird errors
>>> like this
>>> upon enabling an addon a second time: http://www.pasteall.org/16500
>>
>> Enabling an addon a second time, if it defines custom properties, will rarely if not never work properly (at this time). It barely has anything to do with autoregistration (though, of course, I understand your frustration. I had to go through the same thing with netrender, so you can't in good faith say that I don't see where you're coming from).
>>
>>> (Hint: after randomly trying things with no particular logic
>>
>> That's something I don't understand. Instead (or while) trying stuff at random, did you send an error report to anyone indicating that there might be something wrong there or at the very least, that the correct way to do things should be documented correctly and in an accessible location?
>>
>> [/me gets on a soap box]
>>
>> (I definitely don't want to single you out in this, so take from what follows only what applies)
>>
>> The API is still in stabilizing phase, if something (anything) doesn't seem right now is THE BEST time to talk about it, not three years from now.
>>
>> Assuming that we know of the issue is the best way to have it falls through the cracks.
>>
>> It's safer to have to repeat to people that we are aware of an issue then to have it go unnoticed, however self-evident is might seem to you.
>>
>> [/me gets down from the soap box]
>>
>>> I discovered that if I move my ID property class definitions
>>> into the
>>> register function, some--not all--of these errors go
>>> away.  But why
>>> that is?  No idea.  It's not intuitive at all,
>>> and therefore a
>>> crap-shoot to solve.  And it's also patently silly
>>> that I have to move
>>> those class definitions there at all.)
>>
>> You shouldn't have to move the IDPropertyGroup classes in the Register function. I didn't have to do it for netrender and reloading the modules works fine (minus the properties issue).
>>
>>> It removes a _trivial_ amount of boiler plate code.
>>> One
>>> register/unregister call per class is not a lot of code.
>>
>> It's not a matter of whether there's little or a lot of boiler plate code. By definition, boiler plate code is something that we should strive to remove. This isn't just a matter of good practices, it minimizes occurrences of errors in the real world.
>>
>>> > as well as the possibility of easier debugging in
>>> those cases where
>>> > registration fails and gives a easy way internally to
>>> figure out what
>>> > is defined where.
>>>
>>> It is still quite easy to find classes with registration
>>> problems,
>>> because they are referenced by name in the explicit
>>> registration
>>> calls.  If one of my panel classes fails to register,
>>> it is not going
>>> to be at all difficult to track down.
>>>
>>> Unless I'm misunderstanding what you mean?
>>
>> I'm talking about debugging RNA registration code, on the C side, something not many people have to do but it helps a lot that it's easier (at least the last time I had to debug in there).
>>
>>> > it ensures that every script is a "good citizen" and
>>> correctly
>>> > registers and unregisters RNA types when the system
>>> needs
>>> > and not when bad code mistakingly does it
>>>
>>> Sure, I can see garbage collection parallels, for
>>> example.  And I
>>> agree that this is nice.
>>
>> It should also be done across the board, for properties too.
>>
>> If your addon only defines operators or panels, everything is peachy.
>>
>>> But what I dislike about it is that it forces the scripter
>>> to
>>> structure their code in specific ways (a subset of what
>>> python allows,
>>> see your mixin comment for example) that may not be most
>>> appropriate
>>> to the problem at hand, or to their preferences.
>>> _And_ more
>>> importantly it introduces a new set of (non-obvious) rules
>>> that the
>>> scripter has to be aware of beyond just python itself and
>>> the blender
>>> python API.
>>
>> The mixin example is not unheard off in the Python ecosystem. It's basically the recommended solution to defining shared behaviors (see how HTTPServer works and other frameworks in the standard libs).
>>
>>> > The rule is that only valid well defined classes
>>> should derive from RNA
>>> > types, so if you have common behaviors to define, use
>>> mixin classes.
>>>
>>> But this is a silly and needless restriction.
>>
>> On the contrary. Having half-defined subclasses of RNATypes breaks the Liskov substitution principle. The verification process done in registration, in essence, is something that you should be doing anyway if you don't want your defined types to be messed up [1]
>>
>> There's no rational way that you can argue against this outside of "but I don't like it".
>>
>> [1] I am fully aware that Python lets you break up your type hierarchy left and right and the only thing you'll get as punishment is a runtime error. That is certainly not a very good way to argue that this is a good thing.
>>
>>>  At the _very least_, if
>>> we stick with auto-registration we ought to do something
>>> like using
>>> decorators to specify what gets registered.  Something
>>> like:
>>>
>>> @bpy.register
>>> class MyPanel(bpy.types.Panel):
>>>     blah blah blah
>>>
>>> At least that way the scripter explicitly knows what is
>>> going to get
>>> registered, and can control that.  It would also
>>> remove some of the
>>> confusing "magical" feeling of the whole thing, and would
>>> allow people
>>> to structure their code as desired/needed.
>>
>> The only difference with the current system is that you need to manually add a decorator to specify something that you already did by deriving from an RNAType (in a metaphorical sense, it's like you telling blender: "Here, this thing is a new RNAType, name Foo, methods bar, ... And btw, don't forget it's an RNAType, so register it"). One thing deriving from this is that with a decorator, you can skip the later, which means that your RNAType class hierarchy now contains something that Blender doesn't even consider (internally) a full RNAType (which, as far as we know, doesn't currently create any issue, but is not something I'd swear is totally stable).
>>
>>
>> === TL/DR ===
>>
>> Simple rules:
>>
>> 1: Only derive classes from RNATypes when they fulfill the RNATypes restrictions.
>> 2. Only add RNA properties in the Register function.
>>
>> Notes:
>> About 1, this covers adding the needed methods and class attributes after definition (whether creating the class with a call to type() or not). Don't do that, you're defining a class that doesn't fulfill its base class contract and then try to fix it after the fact.
>>
>> About 2, RNA properties definition can cause issues if registering a module multiple time (whether with the reload all functionality or by unloading and reloading an addon). This is a known issue.
>>
>> That was certainly much much longer than I originally intended. Apologies for that.
>>
>> Martin

Hi, I have thought about this a bit but wanted to wait a bit to see if
there was any other user feedback.

Originally I was not happy to accept this meta-classing patch but
mainly because the API had bugs I wanted fixing before adding more
complexity.

Martin was very keen to commit this and Brecht and Andrea also thought
this was good to add, so I agreed it was OK but still had
reservations, along the same lines as Nathans first comments.

But now its become a problem that we have more complicated situations
which I dont think can be handled well automatically...

1) registering classes which have properties which are themselves are
rna classes... and so on.
2) registration order in these complex cases is not clear.
3) unregistering these needs to be done in the right order or there
are problems.
4) dynamically defined classes/operators are also a case where
auto-registration becomes more tricky then its worth.

Reloading addons/scripts and all of the above problems IMHO mean that
automatic registration is just not able to deal with this well.

For the most part is works OK but there are still problems. simple
example is bug.
https://projects.blender.org/tracker/index.php?func=detail&aid=24132&group_id=9&atid=498
I tried for an hour or so to fix it but its just not easy to do with
the current system.

I agree with Nathan that this behavior is mysterious.
In simple cases its convenient but for complex cases the script author
has no idea whats going on and ends up needing to read blenders
internal registration code to figure out how to make their script
work.

IMHO this is a case where our API tries to hide inner working where it
shouldn't.

So I'm proposing to remove auto registration, will wait until next
meeting to decide.

For situations where auto-registration makes sense we can have some
opt-in meta-class.

-- 
- Campbell