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

Trying to fix API doc signatures #150

Merged
merged 16 commits into from Sep 9, 2022
Merged

Trying to fix API doc signatures #150

merged 16 commits into from Sep 9, 2022

Conversation

CodyCBakerPhD
Copy link
Member

Curious if this fixes #147...

The __getattribute__ override itself shouldn't have changed anything, but the metaclass inheritance may have confused sphinx on where to look for the class arguments.

This was apparently working back in the day (link from an old PR): https://neuroconv--56.org.readthedocs.build/en/56/api/interfaces.ecephys.html#module-neuroconv.datainterfaces.ecephys.ced.ceddatainterface

@CodyCBakerPhD
Copy link
Member Author

I know what the problem is now - it's because of the __new__ usage, which sphinx probably looks at as defining the class instead of __init__ whenever it's set. But the __new__ is only defined once, generally, so it needs dynamic inputs.

Looking into how to tell sphinx to just look at the __init__ and ignore __new__

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review September 8, 2022 21:20
@CodyCBakerPhD
Copy link
Member Author

OK This is fixed now.

What I say above about the introduction of the BaseExtractorInterface.__new__ override being the source of the issue is half-true, though the actual effect on readthedocs is a tad more nuanced and was pretty confusing. The actual source issue and solution seems to be Python version dependent (or specifically, version dependent on the inspect submodule of Python standard).

Basically, autodoc draws its default signatures from a function like this (or exactly this, if using that exact version of sphinx): https://github.com/sphinx-doc/sphinx/blob/b4eca324b0499a4c41ba75b45dedb3d8d7e4497f/sphinx/ext/autodoc/typehints.py#L15-L34

As you can see, it basically just looks at the inspect.signature of the class and adds a bit of their own stringification on top of it.

Now, what is weird is when I called inspect.signature on our new extractor-based data interfaces from my typical 3.9 testing environment, what returned looked perfectly fine. Had all the same fields and annotations as the corresponding __init__, as desired.

However, trying it in a lower environment replicates what readthedocs sees (and unfortunately, I don't see an option to force readthedocs to use anything other than 3.8...) which is instead, inspect.signature applies to the __new__ of the class, which because we define it so generally for code reduction, has only *args and **kwargs. I tracked down some of these discussions and changes here: python/cpython#85074 but not 100% if any of that directly altered the state of things here without further combing their changelog.

That's all for background. TL;DR - we can fix this real easy with a couple lines in the conf.py that uses a slight override to the sphinx app to tell it to directly draw class signatures from the class' __init__.

@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #150 (feaa3ac) into main (af3557f) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #150   +/-   ##
=======================================
  Coverage   91.51%   91.51%           
=======================================
  Files          64       64           
  Lines        3349     3349           
=======================================
  Hits         3065     3065           
  Misses        284      284           
Flag Coverage Δ
unittests 91.51% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...onv/datainterfaces/ecephys/ced/ceddatainterface.py 85.71% <100.00%> (ø)

@h-mayorquin
Copy link
Contributor

Nice. I was expecting this would solve the issue on #148 but the tools do not really show up here either. I am starting to think it might be an import issue? I wonder if you have any ideas there.

Btw, #149 has the configuration to use python 3.9 on read the docs but I still don't see proper signatures. Check out the .readthedocs.yaml file, it has an option for that.

Finally, where did you find this setup function for overiding conf.py, some quick googling did not yield anything good.

@CodyCBakerPhD
Copy link
Member Author

Btw, #149 has the configuration to use python 3.9 on read the docs but I still don't see proper signatures. Check out the .readthedocs.yaml file, it has an option for that.

So weird... Maybe it's a specific 3.9 subversion? I have no idea at that point.

Finally, where did you find this setup function for overiding conf.py, some quick googling did not yield anything good.

You see stuff 'like it' scattered around any time custom stuff is needed. I've used it for other stuff before just not this particular function.

Here's one example: https://stackoverflow.com/questions/5599254/how-to-use-sphinxs-autodoc-to-document-a-classs-init-self-method

The rest is digging into the Python source code for sphinx.

@CodyCBakerPhD CodyCBakerPhD merged commit 1c6067e into main Sep 9, 2022
@CodyCBakerPhD CodyCBakerPhD deleted the try_fix_147 branch September 9, 2022 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Signature of interfaces not propagated to the documentation API
2 participants