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

Reworked the neuron.hclass3 module #1096

Merged
merged 31 commits into from
Apr 16, 2021

Conversation

Helveg
Copy link
Contributor

@Helveg Helveg commented Mar 20, 2021

I rewrote the hclass3 module as it is the most important link and entry-point into the hoc module for object oriented Python developers that want to understand where their HOC objects are coming from. In its original form this was quite hard to figure out and it was using some outdated/problematic idioms (see #1092, #1094, #1093) mostly only of importance once people start actually creating and inspecting subclasses of the Python interface objects.

I unpacked the strings in hclass2 and hclass3 (closes #1092). I added a factory class that fills in the required class module attributes for inspection (closes #1094). I switched out the local hc class for a better nonlocal HocBaseObject that does not require a metaclass (closes #1093) and added a PEP8 compliant __all__ attribute. I also think, hclass3.py should be renamed to _hclass3.py as it is an internal interface, but that will be part of another, larger move toward a PEP compliant neuron package in general ;).

(For clarity and PEP compliance I did rename the .htype attribute to ._hoc_type, but I think it's only used to pass along the hoc type to the __new__ function anyway)

I added RST docstrings to all the function and added a dev comment for anyone that would like to know how their HOC object types are generated. I also made sure that all invalid use of the functions resulted in clear exceptions raised where they occurred.

@Helveg Helveg changed the title Reworked the hclass3 module Reworked the neuron.hclass3 module Mar 20, 2021
@codecov-io
Copy link

codecov-io commented Mar 20, 2021

Codecov Report

Merging #1096 (bdff470) into master (42bd79d) will increase coverage by 0.04%.
The diff coverage is 40.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1096      +/-   ##
==========================================
+ Coverage   31.42%   31.47%   +0.04%     
==========================================
  Files         571      572       +1     
  Lines      108767   108863      +96     
==========================================
+ Hits        34182    34261      +79     
- Misses      74585    74602      +17     
Impacted Files Coverage Δ
share/lib/python/neuron/hclass2.py 0.00% <0.00%> (ø)
share/lib/python/neuron/hclass35.py 0.00% <0.00%> (ø)
share/lib/python/neuron/tests/_subclass.py 0.00% <0.00%> (ø)
share/lib/python/neuron/__init__.py 27.89% <60.00%> (+0.33%) ⬆️
share/lib/python/neuron/hclass3.py 70.21% <70.45%> (-29.79%) ⬇️
src/parallel/bbssrv2mpi.cpp 54.59% <0.00%> (-2.30%) ⬇️
src/parallel/bbsclimpi.cpp 51.29% <0.00%> (+1.29%) ⬆️
src/nrnpython/nrnpy_hoc.cpp 56.00% <0.00%> (+2.78%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42bd79d...bdff470. Read the comment docs.

@ramcdougal
Copy link
Member

Regular tests go in /test.

@Helveg Helveg marked this pull request as draft March 20, 2021 17:50
@Helveg
Copy link
Contributor Author

Helveg commented Mar 20, 2021

I think it also closes #771 (as we can use trivial noop class mixins for hierarchy) and #784 (the extended discussion; as we can control the public API a lot better from these neuron.* objects)

@pramodk
Copy link
Member

pramodk commented Mar 20, 2021

thanks @Helveg! 👌 (this is in draft. once ready for review, tag Michael and Robert for the review)

if len(hoc_bases) > 1:
bases = ", ".join(b.__name__)
cname = cls.__name__
raise TypeError(f"Composition of {bases} HocObjects not allowed in {cname}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: f-strings require Python 3.6+

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see... Python 3.5 goes through a different file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, Python 3.5 or earlier just keeps using the old hclass3.py file since the required improvements to metaclasses were simply not available in earlier versions, so I kept things the same there.

@ramcdougal
Copy link
Member

In order for hclass35.py to be copied into place with an autotools-based installation (which we're deprecating, but that means it's still supposed to work), it needs to be mentioned in the Makefile.am in share/lib/python/neuron

@Helveg
Copy link
Contributor Author

Helveg commented Mar 27, 2021

I added tests and make some minor changes such as allowing composition if the HOC type is the same and requiring __new__ when __init__ is overridden (to make sure users don't run into confusing situations regarding #1129). Tests should cover all responsible adults trying to create their own Python classes from HOC types.

test/pynrn/_pyobj_testing.py Show resolved Hide resolved
@pramodk
Copy link
Member

pramodk commented Apr 1, 2021

Is this becoming ready? In that case, we can Update branch and get it in master?

@Helveg
Copy link
Contributor Author

Helveg commented Apr 1, 2021

It's ready, there's one open action point by @ramcdougal to integrate the benefits of nonlocal_hclass into hclass but I'll do that in another PR.

@pramodk
Copy link
Member

pramodk commented Apr 1, 2021

There are some CI failures but Github has changed some default CI images yesterday/today. And that's why we have been seeing some random failures. Hopefully this will be fixed in coming days.

@ramcdougal : this PR looks good for you as well?

@@ -85,4 +86,3 @@ EXTRA_DIST = $(nobase_neuronhomepython_DATA)
# if test $(NRNPYTHON_PYMAJOR) -gt 2 ; then \
# $(PY2TO3) -w $(DESTDIR)$(neuronhomepythondir) ; \
# fi

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: avoid trivial changes

@@ -1,5 +1,3 @@
## Files which get installed in $(prefix)/share:

SUBDIRS = rxd crxd

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: avoid trivial changes... in this file especially, there's no need to change this file at all

share/lib/python/neuron/__init__.py Outdated Show resolved Hide resolved
share/lib/python/neuron/__init__.py Show resolved Hide resolved
share/lib/python/neuron/hclass3.py Show resolved Hide resolved
share/lib/python/neuron/tests/_subclass.py Show resolved Hide resolved
test/pynrn/_pyobj_testing.py Show resolved Hide resolved
@pramodk
Copy link
Member

pramodk commented Apr 16, 2021

@Helveg : would love to get this PR ready with 8.0 release. @ramcdougal added comments but I see that those are now marked as resolved as well. Anything else is pending here? 🚀

@Helveg
Copy link
Contributor Author

Helveg commented Apr 16, 2021

There's still some things to be done, I'll get to that right away!

@Helveg
Copy link
Contributor Author

Helveg commented Apr 16, 2021

@pramodk I have adressed all the review comments, but I can't see the build error for 987e799 Azure pipeline neuronsimulator.nrn — #20210416.14 failed. Oh anyway, that seems to have been a one-off thing, as b6d25bc passed the Azure pipeline

@ramcdougal ramcdougal merged commit f14fad3 into neuronsimulator:master Apr 16, 2021
@pramodk
Copy link
Member

pramodk commented Apr 16, 2021

🎉 thank you @Helveg !

@alexsavulescu : please cherry-pick this for 8.0!

@Helveg Helveg deleted the hclass-nometa-nonlocal branch April 16, 2021 16:39
alexsavulescu pushed a commit that referenced this pull request 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 pull request 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
@alexsavulescu alexsavulescu mentioned this pull request Mar 22, 2022
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants