-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
DOC: use Sphinx extension directly from numpy/ipython instead of maintaining our own version #5221
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
Comments
@jorisvandenbossche this all sounds great.....+1 to decrease these types of deps |
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. |
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). 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) |
I'd rather have it be an external dep or have it checked into pandas proper. Submodules can be obnoxious. |
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. |
@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) |
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. |
@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. |
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 |
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 ... |
Also changed the setting for the IPython cache too right? |
I gave this a first try, so just changing in the 'extensions' list in conf.py the following two lines:
with
The issues I encountered which should be solved:
|
What version of IPython are you using for this? |
@jtratner IPython master, but I think that is not that different from 1.0 |
I'd like to get the essential changes in from pandas and statsmodels into ipython/ipython#4570.
|
@chebee7i About point 3, the |
I rebased all the tweaks including okexcept on top of latest ipython version, but the cython |
@jorisvandenbossche are we tabling this? |
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. |
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. |
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 ipythonsipython_directive
andipython_console_highlighting
(also matplotlibsplot_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:
Possible problems with this is that the version in the pandas repo has converged from the version in numpy/ipython:
numpydoc
this will not be a problem. Following the history it was updated 3 years ago, and then only recently for python 3 compatibility.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.The text was updated successfully, but these errors were encountered: