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

DOC: use Sphinx extension directly from numpy/ipython instead of maintaining our own version #5221

Closed
jorisvandenbossche opened this issue Oct 14, 2013 · 20 comments · Fixed by #6441
Labels
Milestone

Comments

@jorisvandenbossche
Copy link
Member

Summary: proposal to not maintain our own version of Sphinx extension in pandas repo but instead use the extensions directly from the project it maintains.

The pandas docs are using some Sphinx extensions that are not included in Sphinx itself: numpydoc and ipythons ipython_directive and ipython_console_highlighting (also matplotlibs plot_directive is imported in conf.py, but this is not included in the pandas repo).
These extension are located here: https://github.com/pydata/pandas/tree/master/doc/sphinxext.

I think that ideally pandas does not have to care about them, but just uses them by importing the extension from the respective project it lives under (numpy/ipython).
Some recent issue with the doc building also arised form outdated versions of these extensions in pandas repo (see part of the commits in PR #5160 and #4165).

The original extensions are living here:

Options we could do:

  • ipython extensions can just be imported from there in conf.py (because ipython is already a dependency)
  • numpydoc could be added as a submodule (this is how eg numpy itself does it: https://github.com/numpy/numpy/tree/master/doc, 'sphinxext' links to numpydoc), or it can be added as a doc build dependency.

Possible problems with this is that the version in the pandas repo has converged from the version in numpy/ipython:

  • I think for numpydoc this will not be a problem. Following the history it was updated 3 years ago, and then only recently for python 3 compatibility.
  • For ipython_directive this seems more of a problem, looking at the history: https://github.com/pydata/pandas/commits/master/doc/sphinxext/ipython_directive.py there seems to be work done on this specifically for the pandas docs. If this is the case that is has converged from the ipython one, then ideally this could be contributed back to ipython (if the feature is not available there), but this will be more work to adapt.
@jreback
Copy link
Contributor

jreback commented Oct 14, 2013

@jorisvandenbossche this all sounds great.....+1 to decrease these types of deps

@jtratner
Copy link
Contributor

I'm fine with this change and if we can rely on ipython and numpy that sounds good to me. The only problem that I see is that this potentially ties us to specific numpy and IPython versions. When you're changing this, can you make sure that the builds pass (i.e., at least build even if they generate warnings/failures because of numpy compat) with numpy 1.6, numpy 1.7, the numpy 1.8rc and numpy master, plus IPython 1.0 and IPython master. (maybe IPython 0.9?). We don't have to add them to Travis, but I want to make sure that we're not entering some new issue with doc building.

@jorisvandenbossche
Copy link
Member Author

I think this can be an advantage of adding it as a submodule (although I don't really have experience with it), as you can include it at a certain commit (and then periodically update it).
Eg for numpydoc it could be done like this. Numpydoc is also splitted into its own repo, so you wouldn't depend on the numpy version.

For ipython it could indeed be more difficult with different version of ipython itself, because it is included in ipython core.

And for clarity, at the moment I just posted it here as an idea to discuss :-) (I have at the moment unfortunately not really the bandwith to look at it)

@jtratner
Copy link
Contributor

I'd rather have it be an external dep or have it checked into pandas proper. Submodules can be obnoxious.

@jseabold
Copy link
Contributor

To add some historical perspective. The separate numpydoc is a recent venture, and the numpydoc extension couldn't always be depended on to be available just because numpy was installed. Similarly ipython_directive in IPython was broken for a long time as it wasn't much used and didn't keep up with internal IPython refactorings. It was fixed (and somewhat maintained in statsmodels) outside of IPython for a while, but not so much anymore.

Long story short, the status quo has changed, so I'm +1 on going back to using the 'official' extensions.

@jorisvandenbossche
Copy link
Member Author

@jseabold Thanks for the explanation!

Do you know anything about the work done on the ipyton directive in pandas? (see the history: https://github.com/pydata/pandas/commits/master/doc/sphinxext/ipython_directive.py, there seems to be some changes done to it)

@jseabold
Copy link
Contributor

No, not since Wes' last changes. If there are issues they should be sent to upstream IPython I think. That's what I'd been doing.

@jorisvandenbossche
Copy link
Member Author

@jseabold The more recent changes are only minor or not relevant I think (mainly py3 compat => ipython now is also compatible, and something copied over from the ipython version), wo maybe they wouldn't cause to much problems when converting to the 'official' extension.
But most of the changes are from the last change of Wes and before (mainly work on the pure python code blocks it seems). Do you know about that? Are they in upstream?

@jseabold
Copy link
Contributor

I couldn't say for sure without looking. These are the only places this would've changed though other than pandas.

https://github.com/ipython/ipython/blob/master/IPython/sphinxext/ipython_directive.py
https://github.com/statsmodels/statsmodels/blob/master/docs/sphinxext/ipython_directive.py

@cpcloud
Copy link
Member

cpcloud commented Oct 22, 2013

I wrote a hack to allow Cython to work, I think there's one other thing, but I can't remember exactly what it is. I could potentially submit a PR to IPython to put the Cython hack in ...

@jtratner
Copy link
Contributor

Also changed the setting for the IPython cache too right?

@jorisvandenbossche
Copy link
Member Author

I gave this a first try, so just changing in the 'extensions' list in conf.py the following two lines:

'ipython_directive',
'ipython_console_highlighting',

with

'IPython.sphinxext.ipython_directive',
'IPython.sphinxext.ipython_console_highlighting',

The issues I encountered which should be solved:

  • the numbering of the ipython code snippets don't start at 1 (so it seems the 'suppressed' lines are counted as well)
  • in "enhancing performance" -> errors with the cython code (as the directive was adapted then to let this build: 8d834fd)
  • unknown option: "okexcept" in ipython directive
  • There goes something wrong with indented lines in pure python code blocks (eg in for loops or function definitions). I suppose IPython's version cannot handle this, and this was introduced in pandas here: 0bc7fec. Although this is used only once in the pandas docs: in faq.rst L84, but it causes the rest of the docs to build incorrectly.
  • There is now also on 'Out' prompt, but it is al little bit off (not aligned with the 'In' prompt)

@jtratner
Copy link
Contributor

What version of IPython are you using for this?

@jorisvandenbossche
Copy link
Member Author

@jtratner IPython master, but I think that is not that different from 1.0

@chebee7i
Copy link
Contributor

I'd like to get the essential changes in from pandas and statsmodels into ipython/ipython#4570.

  1. It should be straightforward to add an option which makes suppressed lines not increment the execution count. I will add this for sure.
  2. Given that both pandas and statsmodels have made changes to how pure python code is parsed, its not really clear to me which version is best. Both claim to be better than what was in the original, but there's not much documentation (aside from examining the code in detail) on what problems were fixed. Which should be used?
  3. Can someone explain the intent behind okexcept? If the @doctest pseudo-decorator is not used, then the code is free to raise an exception anyway.

@jorisvandenbossche
Copy link
Member Author

@chebee7i About point 3, the okexcept option. As far as I understand, this parameter is used when the code is intended to raise an exception (you want to show this in the docs), so you don't want to show up during the doc builds (because warnings/exceptions that are raised during the execution of the code in the ipython directive are also sent to the stdout (at least in the pandas version of the directive)). So you know that all warnings/exceptions you see in the console while building the docs is something you have to deal with.

@ghost
Copy link

ghost commented Jan 13, 2014

I rebased all the tweaks including okexcept on top of latest ipython version, but the cython
magic snippets don't work with the latest version. I opened an issue.

ipython/ipython#4791
ipython/ipython#4803

@jreback
Copy link
Contributor

jreback commented Feb 18, 2014

@jorisvandenbossche are we tabling this?

@jorisvandenbossche
Copy link
Member Author

Status:

I will do a quick check if the ipython_directive is now in line with ipython's version, and then put in a README explaining this are copies of upstream versions, and changes should be if possible done there, and then we can close this.

@jorisvandenbossche
Copy link
Member Author

It seems that only the latest change of @y-p to fix the unicode issue (#6185) is not yet pushed upstream to ipython.

But I think we decided on keeping our local copy, but trying to the extent possible to push fixes upstream instead of adapting our local version.
I will do a PR that documents this decision to close this PR.

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

Successfully merging a pull request may close this issue.

6 participants