Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HocObject.__new__ requires constructor arguments, HocObject.__init__ should do that. #1129

Open
Helveg opened this issue Mar 27, 2021 · 11 comments
Assignees
Labels

Comments

@Helveg
Copy link
Contributor

Helveg commented Mar 27, 2021

In Python __new__ and __init__ have 2 distinct purposes: __new__ creates an instance, usually of the same type as the type object and returns it, while __init__ initializes instances of the type object, this means that __new__ should only use the arguments it is given to help it determine the type of the instance to create, but should not use them to initialize the created object.

HocObject.__new__ however does jump the gun there: it creates an instance of the type object (determined by the hocbase kwarg) but also requires the constructor arguments of the HOC type and uses them to construct a fully initialized object.

This leads to a problem in inheritance where a class that inherits from h.List cannot rewrite its __init__ arguments to be anything other than the arguments expected by h.List, since the arguments are first passed to __new__ and need to match the arguments HocObject.__new__ require to construct a h.List object: the h.List arguments, QED.

The only way around this problem is for child classes that implement __init__ to also implement __new__ and to rewrite the args they pass into super().__new__.

The solution should be that __new__ only does a placeholder construction of the correct type, but waits for the arguments in HocObject.__init__ before finishing construction of the object.

@nrnhines
Copy link
Member

Would it make sense to repair the new/init pairs in nrnpy_hoc.cpp and nrnpy_nrn.cpp to conform to standard python policy?

ramcdougal pushed a commit that referenced this issue Apr 16, 2021
* added pyenv's .python-version to gitignore

* prevent import py2/3 module in py3/2. removed `exec` wrapper from hclass

* Changed comment and removed non-existing `nonlocal_hclass` import

* Removed traling whitespaces and double newline at eof

* Removed the MetaHocObject metaclass and added `nonlocal_hclass` factory

* New `HocBaseObject`; (nonlocal_)hclass uses this new base.

* Added dev note to demystify quintessential link between HOC & Python

* moved __all__ to PEP8 location

* Fixed error with grandchildren of `HocBaseObject`

* Apply changes only to Python 3.6+ where metaclass alternative is availbl

* fixed import typo

* hclass of hoc_type should precede use of hoc.HocObject API's

* Fixed usage of module_name

* added hclass35.py to Makefile, excessive trailing newlines.

* `module` does not exist, see details on mixed approaches in PR#1096

* black formatting & updated hclass2 derived docstrings

* Store dummy modules on `neuron` module to better emulate submodules

* Add a TypeError if init but not new has been given (see #1129)

* Added _pyobj_enabled flag to check whether Python object interface is up

* Explicitly  defined __new__ for 3.6+ pyobj compatibility

* Class composition is allowed if HOC types are the same

* added tests for HOC type Python classes, test only in Python 3.6+

* Removed 'super()' call for Python 2

* Revert "added hclass35.py to Makefile, excessive trailing newlines."

This reverts commit 080f14a.

* Added hclass35 to Makefile

* Revert "Removed traling whitespaces and double newline at eof"

This reverts commit 046fab4.

* Reinstated deprecation messages

* Merged hclass and nonlocal_hclass
alexsavulescu pushed a commit that referenced this issue Apr 19, 2021
* added pyenv's .python-version to gitignore

* prevent import py2/3 module in py3/2. removed `exec` wrapper from hclass

* Changed comment and removed non-existing `nonlocal_hclass` import

* Removed traling whitespaces and double newline at eof

* Removed the MetaHocObject metaclass and added `nonlocal_hclass` factory

* New `HocBaseObject`; (nonlocal_)hclass uses this new base.

* Added dev note to demystify quintessential link between HOC & Python

* moved __all__ to PEP8 location

* Fixed error with grandchildren of `HocBaseObject`

* Apply changes only to Python 3.6+ where metaclass alternative is availbl

* fixed import typo

* hclass of hoc_type should precede use of hoc.HocObject API's

* Fixed usage of module_name

* added hclass35.py to Makefile, excessive trailing newlines.

* `module` does not exist, see details on mixed approaches in PR#1096

* black formatting & updated hclass2 derived docstrings

* Store dummy modules on `neuron` module to better emulate submodules

* Add a TypeError if init but not new has been given (see #1129)

* Added _pyobj_enabled flag to check whether Python object interface is up

* Explicitly  defined __new__ for 3.6+ pyobj compatibility

* Class composition is allowed if HOC types are the same

* added tests for HOC type Python classes, test only in Python 3.6+

* Removed 'super()' call for Python 2

* Revert "added hclass35.py to Makefile, excessive trailing newlines."

This reverts commit 080f14a.

* Added hclass35 to Makefile

* Revert "Removed traling whitespaces and double newline at eof"

This reverts commit 046fab4.

* Reinstated deprecation messages

* Merged hclass and nonlocal_hclass
alexsavulescu pushed a commit that referenced this issue Apr 30, 2021
* added pyenv's .python-version to gitignore

* prevent import py2/3 module in py3/2. removed `exec` wrapper from hclass

* Changed comment and removed non-existing `nonlocal_hclass` import

* Removed traling whitespaces and double newline at eof

* Removed the MetaHocObject metaclass and added `nonlocal_hclass` factory

* New `HocBaseObject`; (nonlocal_)hclass uses this new base.

* Added dev note to demystify quintessential link between HOC & Python

* moved __all__ to PEP8 location

* Fixed error with grandchildren of `HocBaseObject`

* Apply changes only to Python 3.6+ where metaclass alternative is availbl

* fixed import typo

* hclass of hoc_type should precede use of hoc.HocObject API's

* Fixed usage of module_name

* added hclass35.py to Makefile, excessive trailing newlines.

* `module` does not exist, see details on mixed approaches in PR#1096

* black formatting & updated hclass2 derived docstrings

* Store dummy modules on `neuron` module to better emulate submodules

* Add a TypeError if init but not new has been given (see #1129)

* Added _pyobj_enabled flag to check whether Python object interface is up

* Explicitly  defined __new__ for 3.6+ pyobj compatibility

* Class composition is allowed if HOC types are the same

* added tests for HOC type Python classes, test only in Python 3.6+

* Removed 'super()' call for Python 2

* Revert "added hclass35.py to Makefile, excessive trailing newlines."

This reverts commit 080f14a.

* Added hclass35 to Makefile

* Revert "Removed traling whitespaces and double newline at eof"

This reverts commit 046fab4.

* Reinstated deprecation messages

* Merged hclass and nonlocal_hclass
@Helveg
Copy link
Contributor Author

Helveg commented Feb 11, 2022

Hi @nrnhines I'm not familiar with the CPP part of NEURON so I can't really answer that. In Python I would solve it by implementing a __init_subclass__ method that sets the __call__ of the class object (or make a metaclass):

class BaseHocObject(HocObject):
 def __init_subclass__(cls, hoctype=None, **kwargs):
  super().__init_subclass__(**kwargs)
  def call(*args, **kwargs):
   # Call the hoc object factory without args
   instance = cls.__new__(hoctype)
   # Pass the args to init
   instance.__init__(*args, **kwargs)
   # Check that the user called `super().__init__()` with the expected args with a flag
   if not getattr(instance, "_hoc_init_passed", False):
    raise Exception("Python HOC objects must call `super().__init__()`)
  
  cls.__call__ = call

This way HOC can always instantiate the right data structure without any args, and by setting a flag on the object we can confirm that the object has been properly initialised! So:

  • HocObject.__new__(type) creates the data structure
  • HocObject.__init__(instance, args, it, has="already") does the initialization of the hoc data structure the same way it used to, and sets the "properly initialized" flag

@nrnhines
Copy link
Member

I don't have a good enough understanding of python capi new/init implementation standards to safely refactor the existing version. I do remember being quite puzzled about when init was actually used. For example, presently, PyObject* NPySecObj_new is itself
calling a quite elaborate int NPySecObj_init and I can't imagine it being a valid nrn.Section if it doesn't. I think I need an exclusive block of time to look into this, study the capi documents in this regard, and examine printf output about when new and init are used. I don't see such a block opening any time soon. I'd be delighted to get advice (@ramcdougal ?) as well. It may very well be the case that a proper refactoring will ramify to other places where mistakes in new/init were worked around and then become mistakes in their own right.

@Helveg
Copy link
Contributor Author

Helveg commented Feb 11, 2022

The basic type metaclass of objects in Python handles new and init like this: init is called after new if new produces an instance of the class that it was handed, and doesn't if it's of another type. eg, when HocObject(*args, **kwargs) is called, as an instance of the metaclass type (the default metaclass), type.__call__(HocObject, *args, **kwargs) is called (with said new/init logic in it). If HocObject.__new__ returns an object of type OtherType, type won't call __init__ on that instance.

PyObject* NPySecObj_new is itself calling a quite elaborate int NPySecObj_init and I can't imagine it being a valid nrn.Section if it doesn't.

It needn't be: An unitialized section isn't a valid Section, that's entirely correct to note that. But it isn't new that should be calling init, it should be whatever is calling new, that should then after new call init. And the __call__ of the metaclass (usually type) is who does that. That's why child classes always have to call the super's init if they override it. We ought to replace the metaclass of HocObjects (or replace the __call__ method) and then write more appropriate new/init logic. And we could detect the lack of a properly initialised flag and raise an error as a courtesy to anyone trying to subclass NEURON objects, should they forget.

It's definitely not the way it usually goes in OO languages, but it is what it is. If new expects too many strict arguments, init is kind of blocked from being meaningfully overridden

@nrnhines
Copy link
Member

Thanks. That's very helpful. Is the properly initialised flag something automatic? I see that NPySecObj_init is only being called once when s = h.Section() and I certainly set no such flag. i.e. after uncommenting the printf in the init/new methods I see:

>>> s = h.Section()
NPySecObj_new 0x555ce902ccc0
NPySecObj_init 0x555ce902ccc0 (nil)

@Helveg
Copy link
Contributor Author

Helveg commented Feb 11, 2022

No, it's a manual addition of this check I proposed:

   if not getattr(instance, "_hoc_init_passed", False):
    raise Exception("Python HOC objects must call `super().__init__()`)

we'd set that on all of the HocObjects whose HocObject.__init__ got properly called, and we'd check it from the metaclass, and throw an error if it was never set. It would help people prevent breaking the inheritance chain of super().__init__s that eventually end up at HocObject.__init__ where we can finalize the object. Take this scenario:

class My100BigVector(HocObject, hocbase=h.Vector):
  def __init__(self, *args):
    super().__init__(100)
    self.args = args

because super().__init__(100) is called, a call to HocObject.__init__ is made, which can now take the arguments needed to finalize the underlying h.Vector object with size 100, and it should also set self._hoc_init_passed=True, so that the metaclass can verify that the proper finalization has occurred after calling __init__

@Helveg
Copy link
Contributor Author

Helveg commented Feb 11, 2022

The reason why both init and new are called here (if it were pure Python objects, the C thing makes me unsure):

>>> s = h.Section()
NPySecObj_new 0x555ce902ccc0
NPySecObj_init 0x555ce902ccc0 (nil)

is because the return value of nrn.Section.__new__(nrn.Section) is of type nrn.Section, satisfying the constraint for init to be called automatically; calling it directly actually produces a funny looking <deleted section>:

>>> a = nrn.Section.__new__(nrn.Section)
>>> type(a)
<class 'nrn.Section'>
>>> a
<deleted section>
>>> a.diam
Segmentation fault

😉

The metaclass of nrn.Section is type which usually means that nrn.Section.__call__ points to type's methods, but it's not the case, probably related to the fact that these objects originate from the C-API and this is where my knowledge ends ;P

>>> nrn.Section.__call__
<slot wrapper '__call__' of 'nrn.Section' objects>

with a regular Python class:

>>> class MyClass: pass
...
>>> MyClass.__call__
<method-wrapper '__call__' of type object at 0x555c97c570a0>
>>> MyClass.__call__()
<__main__.MyClass object at 0x7f0b984589d0>

@Helveg
Copy link
Contributor Author

Helveg commented Feb 11, 2022

Hah, so:

>>> a = nrn.Section.__new__(nrn.Section)
>>> a
<deleted section>
>>> a.__init__()
>>> a
__nrnsec_0x555c98100e40
>>> type(nrn.Section).__call__(nrn.Section)
__nrnsec_0x555c98100ea0

do you see how the nrn.Section.__new__ + nrn.Section.__init__ two-part sequence is equal to type.__call__(nrn.Section)? Because calling a class, creates an instance. (I use type.__call__(nrn.Section) to emulate nrn.Section.__call__() because that one threw an error)

Now the problem is actually restricted to HocObjects, where __new__ expects a strict set of arguments, rather than accepting and ignoring all arguments handed to it, and doing the initialization from __init__:

>>> hoc.HocObject.__new__(hoc.HocObject, hocbase=h.PtrVector)
NEURON: PtrVector not enough arguments
 near line 0
 ^
        PtrVector()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: hoc error

new should only care about the hocbase arg, and produce template HocObjects, and wait for init to finalize them, knowing that it will always be automatically called (expect for in the case of people forgetting to call super() from their subclass, which is why I started the init flag thingy)

@ramcdougal
Copy link
Member

For what it's worth, according to https://llllllllll.github.io/c-extension-tutorial/appendix.html#c.PyTypeObject.tp_new

If the type is immutable, the rest of the initialization should happen in the PyTypeObject.tp_new. If the type is mutable, the initialization should happen in the PyTypeObject.tp_init which will be called automatically.

@nrnhines
Copy link
Member

I guess that pretty much settles the abstract question. The decision then, in each case, is whether the object is mutable or immutable. nrn.Section is immutable in the sense that on instantiation, it points to a NEURON Section and can never be changed to point to another NEURON Section. If the NEURON Section become invalid, then any ```nrn.Section`` that points to it is also invalid. On the other hand there are so many aspects of a NEURON Section that are mutable. I guess that is also the case for many uses of PyHocObject which wrap a Hoc Object.

@Helveg
Copy link
Contributor Author

Helveg commented Feb 18, 2022

Well in Python mutability is only considered in as far as immutability is an obstacle people run into and try to work around ;)

If an object's attributes can be changed after construction it is mutable. The immutable aspect of a Section pointing to a NEURON section is exactly why nrn.Section instances shouldn't be given a .section attribute 🙃 Keep the immutable portion under the hood and everything should be fine for making Python NEURON objects mutable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants