Re: [Freeipa-devel] Plugin registration API (Was: [PATCH] Return klass in api.register())
On 08/05/2013 03:41 PM, Nathaniel McCallum wrote: On Thu, 2013-08-01 at 15:57 +0200, Petr Viktorin wrote: Here is a patch that implements the API I proposed, but with old semantics. Plugins using this won't need to be rewritten when we switch the behavior as well. +1, reviewed. Thanks, pushed to master: 7804a74826eebd1ed07bc9eaa28c1eb9bff5a3f7 I've also converted one of the plugins to use this. If your approach is accepted please do this as well -- if we support an API we want it tested. I prefer your design. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Plugin registration API (Was: [PATCH] Return klass in api.register())
On Thu, 2013-08-01 at 15:57 +0200, Petr Viktorin wrote: > Here is a patch that implements the API I proposed, but with old > semantics. Plugins using this won't need to be rewritten when we switch > the behavior as well. +1, reviewed. > I've also converted one of the plugins to use this. If your approach is > accepted please do this as well -- if we support an API we want it tested. I prefer your design. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Plugin registration API (Was: [PATCH] Return klass in api.register())
On 07/31/2013 06:43 PM, Nathaniel McCallum wrote: Mostly, we're agreed. This was mostly an inventory of approaches I've seen in the Python world before. On Wed, 2013-07-31 at 11:08 +0200, Petr Viktorin wrote: On 07/30/2013 08:05 PM, Nathaniel McCallum wrote: On Tue, 2013-07-30 at 18:58 +0200, Petr Viktorin wrote: On 07/30/2013 05:16 PM, Nathaniel McCallum wrote: From 371e080fb6a86da6f0ec574c8cbc0d8d06c18c99 Mon Sep 17 00:00:00 2001 From: Nathaniel McCallum Date: Tue, 30 Jul 2013 11:13:55 -0400 Subject: [PATCH] Return klass in api.register() This permits the use of api.register as a decorator. Thanks for the patch. Was there any discussion/reason for this? Can you file a ticket? I'm not opposed, I agree this would allow less typing for plugin authors. There was not really any discussion. I just simply noticed that a single line change would make plugins a bit more pythonic. I mentioned it to rcrit and he didn't see any problem with it. So I wrote the patch. Expressing registration as a decorator has numerous advantages, including being less error prone and easier to read. But typing less is probably a good enough reason. :) In my mind, this can be merged now since it doesn't break any backwards compatibility and enables a syntax that is easier to modify in the future. It is the migration from a function to a decorator that is where all the work lies. Once you have decorators, you can fairly easily do a sed replacement (or maintain backwards compatibility if you like). The patch is backwards compatible, but the new syntax is not forward compatible. You can't easily run sed on third-party plugins -- we want to switch just once. We should switch to supporting both the old and new syntax. An intermediate syntax that's better than the old one but not compatible with future plans isn't worth it. I can make a patch that adds my proposed syntax but keeps the old behavior. I'd like to get the design done first though. (And of course, the old syntax and functionality would still be supported) The *existing* syntax is not forward compatible. The current patch permits the use of decorators. It does NOT require the migration to decorators. Any new code that is written with decorators will be easier to migrate to the new API. Hence: 1. Existing code stays the same and still works 2. New code uses the decorator syntax 3. New plugin api forces manual migration from #1 (hard) and #2 (easy) They're actually about the same level of easiness - deleting a line from below a class and adding one above the class is not hard. The main problem here is that someone has to look at the plugin and do the changes, and commit/deploy them. I'd like to skip this problem in the #2 case altogether. There are third-party plugins. We want to make these disruptions as infrequent as possible. The cost/benefit analysis of my patch is: no cost, minor benefit. The only reason I can see not to merge this patch is if we would choose a plugin registration system without decorators. This would make the migration from #2 to #3 harder. Here is a patch that implements the API I proposed, but with old semantics. Plugins using this won't need to be rewritten when we switch the behavior as well. I've also converted one of the plugins to use this. If your approach is accepted please do this as well -- if we support an API we want it tested. I also think the plugin registration API needs a larger redesign, though I haven't shared my thoughts on it lately as there are more pressing things to do. I'll correct that now that there's (sort of) a thread on the topic :) Basically, my gripe is that plugin registration only works on the one global API object, ipalib.api. If you create a different API object, you can't register plugins on it. Different API object would be useful for testing, or for running IPA commands on another master (there's an ugly hack in ipa-replica-install that does that). A rough sketch of what I had in mind would be: plugin.py: # the "register" decorator will collect registered items; # it must be named "register" so the loading routine finds it register = plugable.Registry() @register class obj_mod(LDAPUpdate): plugable.py: class Registry: def __call__(self, item): self.items.append(item) class API: def load_plugin_package(self, package_name): plugin = __import__(package_name).register for item in register.items: self.register(item) Related ticket for this: https://fedorahosted.org/freeipa/ticket/3090 Maybe if we're giving an incentive for plugin authors to rewrite plugins, we should give them an API that's at least compatible with the above goals, so they won't have to rewrite again some time later. Having a registration collector as an independent object makes a lot of sense. I also prefer type-safety to strings. Make sure you make the d
Re: [Freeipa-devel] Plugin registration API (Was: [PATCH] Return klass in api.register())
Mostly, we're agreed. This was mostly an inventory of approaches I've seen in the Python world before. On Wed, 2013-07-31 at 11:08 +0200, Petr Viktorin wrote: > On 07/30/2013 08:05 PM, Nathaniel McCallum wrote: > > On Tue, 2013-07-30 at 18:58 +0200, Petr Viktorin wrote: > >> On 07/30/2013 05:16 PM, Nathaniel McCallum wrote: > >>> From 371e080fb6a86da6f0ec574c8cbc0d8d06c18c99 Mon Sep 17 00:00:00 2001 > >>> From: Nathaniel McCallum > >>> Date: Tue, 30 Jul 2013 11:13:55 -0400 > >>> Subject: [PATCH] Return klass in api.register() > >>> > >>> This permits the use of api.register as a decorator. > >> > >> Thanks for the patch. > >> Was there any discussion/reason for this? Can you file a ticket? > >> I'm not opposed, I agree this would allow less typing for plugin authors. > > > > There was not really any discussion. I just simply noticed that a single > > line change would make plugins a bit more pythonic. I mentioned it to > > rcrit and he didn't see any problem with it. So I wrote the patch. > > > > Expressing registration as a decorator has numerous advantages, > > including being less error prone and easier to read. But typing less is > > probably a good enough reason. :) > > > > In my mind, this can be merged now since it doesn't break any backwards > > compatibility and enables a syntax that is easier to modify in the > > future. It is the migration from a function to a decorator that is where > > all the work lies. Once you have decorators, you can fairly easily do a > > sed replacement (or maintain backwards compatibility if you like). > > The patch is backwards compatible, but the new syntax is not forward > compatible. You can't easily run sed on third-party plugins -- we want > to switch just once. > We should switch to supporting both the old and new syntax. An > intermediate syntax that's better than the old one but not compatible > with future plans isn't worth it. > > I can make a patch that adds my proposed syntax but keeps the old > behavior. I'd like to get the design done first though. > (And of course, the old syntax and functionality would still be supported) The *existing* syntax is not forward compatible. The current patch permits the use of decorators. It does NOT require the migration to decorators. Any new code that is written with decorators will be easier to migrate to the new API. Hence: 1. Existing code stays the same and still works 2. New code uses the decorator syntax 3. New plugin api forces manual migration from #1 (hard) and #2 (easy) The cost/benefit analysis of my patch is: no cost, minor benefit. The only reason I can see not to merge this patch is if we would choose a plugin registration system without decorators. This would make the migration from #2 to #3 harder. > >> I also think the plugin registration API needs a larger redesign, though > >> I haven't shared my thoughts on it lately as there are more pressing > >> things to do. I'll correct that now that there's (sort of) a thread on > >> the topic :) > >> > >> Basically, my gripe is that plugin registration only works on the one > >> global API object, ipalib.api. If you create a different API object, you > >> can't register plugins on it. Different API object would be useful for > >> testing, or for running IPA commands on another master (there's an ugly > >> hack in ipa-replica-install that does that). > >> > >> A rough sketch of what I had in mind would be: > >> > >> plugin.py: > >> # the "register" decorator will collect registered items; > >> # it must be named "register" so the loading routine finds it > >> register = plugable.Registry() > >> > >> @register > >> class obj_mod(LDAPUpdate): > >> > >> > >> plugable.py: > >> class Registry: > >> def __call__(self, item): > >> self.items.append(item) > >> > >> class API: > >> def load_plugin_package(self, package_name): > >> plugin = __import__(package_name).register > >> for item in register.items: > >> self.register(item) > >> > >> Related ticket for this: https://fedorahosted.org/freeipa/ticket/3090 > >> > >> > >> Maybe if we're giving an incentive for plugin authors to rewrite > >> plugins, we should give them an API that's at least compatible with the > >> above goals, so they won't have to rewrite again some time later. > > > > Having a registration collector as an independent object makes a lot of > > sense. I also prefer type-safety to strings. > > > > Make sure you make the decorator a callable though. This permits you to > > easily add options later without breaking backwards compatibility. > > > > You want: > > @register() > > class foo: > >pass > > > > Not: > > @register > > class foo: > >pass > > Fair point. > > So we're at: > from ipalib.plugable import Registry > > register = Registry() > > @register() > class object_mod(...) > ... > > > Also, you can avoid the
Re: [Freeipa-devel] Plugin registration API (Was: [PATCH] Return klass in api.register())
On 07/30/2013 08:05 PM, Nathaniel McCallum wrote: On Tue, 2013-07-30 at 18:58 +0200, Petr Viktorin wrote: On 07/30/2013 05:16 PM, Nathaniel McCallum wrote: From 371e080fb6a86da6f0ec574c8cbc0d8d06c18c99 Mon Sep 17 00:00:00 2001 From: Nathaniel McCallum Date: Tue, 30 Jul 2013 11:13:55 -0400 Subject: [PATCH] Return klass in api.register() This permits the use of api.register as a decorator. Thanks for the patch. Was there any discussion/reason for this? Can you file a ticket? I'm not opposed, I agree this would allow less typing for plugin authors. There was not really any discussion. I just simply noticed that a single line change would make plugins a bit more pythonic. I mentioned it to rcrit and he didn't see any problem with it. So I wrote the patch. Expressing registration as a decorator has numerous advantages, including being less error prone and easier to read. But typing less is probably a good enough reason. :) In my mind, this can be merged now since it doesn't break any backwards compatibility and enables a syntax that is easier to modify in the future. It is the migration from a function to a decorator that is where all the work lies. Once you have decorators, you can fairly easily do a sed replacement (or maintain backwards compatibility if you like). The patch is backwards compatible, but the new syntax is not forward compatible. You can't easily run sed on third-party plugins -- we want to switch just once. We should switch to supporting both the old and new syntax. An intermediate syntax that's better than the old one but not compatible with future plans isn't worth it. I can make a patch that adds my proposed syntax but keeps the old behavior. I'd like to get the design done first though. (And of course, the old syntax and functionality would still be supported) I also think the plugin registration API needs a larger redesign, though I haven't shared my thoughts on it lately as there are more pressing things to do. I'll correct that now that there's (sort of) a thread on the topic :) Basically, my gripe is that plugin registration only works on the one global API object, ipalib.api. If you create a different API object, you can't register plugins on it. Different API object would be useful for testing, or for running IPA commands on another master (there's an ugly hack in ipa-replica-install that does that). A rough sketch of what I had in mind would be: plugin.py: # the "register" decorator will collect registered items; # it must be named "register" so the loading routine finds it register = plugable.Registry() @register class obj_mod(LDAPUpdate): plugable.py: class Registry: def __call__(self, item): self.items.append(item) class API: def load_plugin_package(self, package_name): plugin = __import__(package_name).register for item in register.items: self.register(item) Related ticket for this: https://fedorahosted.org/freeipa/ticket/3090 Maybe if we're giving an incentive for plugin authors to rewrite plugins, we should give them an API that's at least compatible with the above goals, so they won't have to rewrite again some time later. Having a registration collector as an independent object makes a lot of sense. I also prefer type-safety to strings. Make sure you make the decorator a callable though. This permits you to easily add options later without breaking backwards compatibility. You want: @register() class foo: pass Not: @register class foo: pass Fair point. So we're at: from ipalib.plugable import Registry register = Registry() @register() class object_mod(...) ... Also, you can avoid the creation of a registry object for each plugin. But that is exactly what I want to do! I *want* a plugin module to just have a list of things it exports, and leave it to the consumer to load them if it wants to. You can essentially do module cache poisoning to force an alternative registry. This saves some typing and keeps the alternate code in the same scope as the tests for that alternate path... Too much magic. I would really rather have things work as they normally do Python. Module cache poisoning is pretty tough on the reader. The extra line that you have to type keeps things clear and tells the reader where to look to see how things are done. I don't understand what exactly you mean by "alternate code" here? Lastly, you could use class hierarchy introspection to avoid registration altogether. This has the advantage of some natural type safety. Did you mean to look which classes in a module are Plugin subclasses? That would mean all plugins now have to be defined as top-level items in a module -- an unnecessary restriction. Doing anything else than a traditional plugin module would get unnecessarily complicated (e.g. creating plugins dynamically -- who knows wh
Re: [Freeipa-devel] Plugin registration API (Was: [PATCH] Return klass in api.register())
On Tue, 2013-07-30 at 18:58 +0200, Petr Viktorin wrote: > On 07/30/2013 05:16 PM, Nathaniel McCallum wrote: > > From 371e080fb6a86da6f0ec574c8cbc0d8d06c18c99 Mon Sep 17 00:00:00 2001 > > From: Nathaniel McCallum > > Date: Tue, 30 Jul 2013 11:13:55 -0400 > > Subject: [PATCH] Return klass in api.register() > > > > This permits the use of api.register as a decorator. > > Thanks for the patch. > Was there any discussion/reason for this? Can you file a ticket? > I'm not opposed, I agree this would allow less typing for plugin authors. There was not really any discussion. I just simply noticed that a single line change would make plugins a bit more pythonic. I mentioned it to rcrit and he didn't see any problem with it. So I wrote the patch. Expressing registration as a decorator has numerous advantages, including being less error prone and easier to read. But typing less is probably a good enough reason. :) In my mind, this can be merged now since it doesn't break any backwards compatibility and enables a syntax that is easier to modify in the future. It is the migration from a function to a decorator that is where all the work lies. Once you have decorators, you can fairly easily do a sed replacement (or maintain backwards compatibility if you like). > I also think the plugin registration API needs a larger redesign, though > I haven't shared my thoughts on it lately as there are more pressing > things to do. I'll correct that now that there's (sort of) a thread on > the topic :) > > Basically, my gripe is that plugin registration only works on the one > global API object, ipalib.api. If you create a different API object, you > can't register plugins on it. Different API object would be useful for > testing, or for running IPA commands on another master (there's an ugly > hack in ipa-replica-install that does that). > > A rough sketch of what I had in mind would be: > > plugin.py: > # the "register" decorator will collect registered items; > # it must be named "register" so the loading routine finds it > register = plugable.Registry() > > @register > class obj_mod(LDAPUpdate): > > > plugable.py: > class Registry: > def __call__(self, item): > self.items.append(item) > > class API: > def load_plugin_package(self, package_name): > plugin = __import__(package_name).register > for item in register.items: > self.register(item) > > Related ticket for this: https://fedorahosted.org/freeipa/ticket/3090 > > > Maybe if we're giving an incentive for plugin authors to rewrite > plugins, we should give them an API that's at least compatible with the > above goals, so they won't have to rewrite again some time later. Having a registration collector as an independent object makes a lot of sense. I also prefer type-safety to strings. Make sure you make the decorator a callable though. This permits you to easily add options later without breaking backwards compatibility. You want: @register() class foo: pass Not: @register class foo: pass Also, you can avoid the creation of a registry object for each plugin. You can essentially do module cache poisoning to force an alternative registry. This saves some typing and keeps the alternate code in the same scope as the tests for that alternate path... Lastly, you could use class hierarchy introspection to avoid registration altogether. This has the advantage of some natural type safety. Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Plugin registration API (Was: [PATCH] Return klass in api.register())
Petr Viktorin wrote: On 07/30/2013 05:16 PM, Nathaniel McCallum wrote: From 371e080fb6a86da6f0ec574c8cbc0d8d06c18c99 Mon Sep 17 00:00:00 2001 From: Nathaniel McCallum Date: Tue, 30 Jul 2013 11:13:55 -0400 Subject: [PATCH] Return klass in api.register() This permits the use of api.register as a decorator. Thanks for the patch. Was there any discussion/reason for this? Can you file a ticket? I'm not opposed, I agree this would allow less typing for plugin authors. I also think the plugin registration API needs a larger redesign, though I haven't shared my thoughts on it lately as there are more pressing things to do. I'll correct that now that there's (sort of) a thread on the topic :) Basically, my gripe is that plugin registration only works on the one global API object, ipalib.api. If you create a different API object, you can't register plugins on it. Different API object would be useful for testing, or for running IPA commands on another master (there's an ugly hack in ipa-replica-install that does that). A rough sketch of what I had in mind would be: plugin.py: # the "register" decorator will collect registered items; # it must be named "register" so the loading routine finds it register = plugable.Registry() @register class obj_mod(LDAPUpdate): plugable.py: class Registry: def __call__(self, item): self.items.append(item) class API: def load_plugin_package(self, package_name): plugin = __import__(package_name).register for item in register.items: self.register(item) Related ticket for this: https://fedorahosted.org/freeipa/ticket/3090 Maybe if we're giving an incentive for plugin authors to rewrite plugins, we should give them an API that's at least compatible with the above goals, so they won't have to rewrite again some time later. IIRC Jason (the original framework author) did that on purpose so you could just api anywhere and get the registered API. It was a limitation to make certain other things easier. This was also back before the deferred initialization work where creating an api was very expensive, so maybe its less of an issue. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
