Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#9128 closed enhancement (fixed)

Sphinx should be aware of all.py to find its links

Reported by: hivert Owned by: hivert
Priority: major Milestone: sage-5.0
Component: documentation Keywords: Sphinx links
Cc: nthiery, leif, novoselt, mhansen Merged in: sage-5.0.beta8
Authors: Florent Hivert Reviewers: Andrey Novoseltsev, Nicolas M. Thiéry
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11251, #12490, #12572 Stopgaps:

Description (last modified by hivert)

Though sphinx is perfectly working with target in the local module he isn't able to find reference target from other modules even if they are exported in all.py. For example, if I want to link Parent from anywhere but parent.pyx, I have to write the full path (ie. :class:`~sage.structure.parent.Parent`) even if it is imported in my module. I find this extremely annoying since, in the task of improving the category doc, I'm setting up a lot of huge cross references such as:

    :meth:`Algebras.ParentMethods.algebra_generators()
    <sage.categories.algebras.Algebras.ParentMethods.algebra_generators>`.

I would be very happy if I had to write only

    :meth:`Algebras.ParentMethods.algebra_generators`

The following patch should solve this issue

I set up intersphinx so that links to Python's doc are correctly resolved. The patch trac_9128-intersphinx_python_database-fh.patch contains Python's crossref database downloaded from http://docs.python.org/objects.inv I'm also using intersphinx to solve links to the reference manual from the other documents.

New option for docbuild

I added the option --warn-links to the documentation build command as in

    sage -docbuild --warn-links reference htm

When the option is used, Sphinx will issue a warning, whenever a link is not resolved.

Extra features

Moreover, I add a reference target to every automatically created .rst file associated to python source files. It can by used to set-up a link toward the file and thus get the title rather than the module. For example :ref:sage.categories.primer setup a link to the help page with title "Elements, parents, and categories in Sage: a (draft of) primer" rather than sage.categories.primer which you get with the link :mod:sage.categories.primer.

Apply both:

Note: order is irrelevant

Attachments (4)

trac_9128-intersphinx_python_database-fh.patch (394.1 KB) - added by hivert 6 years ago.
trac_9128-doc_option-fh.patch (3.7 KB) - added by hivert 6 years ago.
trac_9128-sphinx_links_all-fh.patch (12.2 KB) - added by nthiery 6 years ago.
trac_9128-MANIFEST-fh.patch (750 bytes) - added by hivert 6 years ago.

Download all attachments as: .zip

Change History (73)

comment:1 Changed 7 years ago by hivert

  • Owner changed from mvngu to hivert

The patch here is a prototype, not yet ready for review. Comments or suggestions are mostly welcome.

comment:2 Changed 7 years ago by hivert

  • Status changed from new to needs_work

comment:3 Changed 7 years ago by leif

  • Cc leif added

comment:4 Changed 7 years ago by hivert

  • Description modified (diff)
  • Status changed from needs_work to needs_review

The submitted patch seems to behave as I want. So I put needs review. However if I receive some good advice on sage-devel or sphinx-users I may still change it. Anyway, Please comment.

comment:5 Changed 7 years ago by novoselt

  • Cc novoselt added

comment:6 Changed 7 years ago by novoselt

I have just upgraded an installation of sage-4.4.2 to 4.4.3, applied this patch, set SAGE_DOC_WARN_DANGLING_LINKS to 1, and then got the following error:

novoselt@sage:/scratch/novoselt/sage/devel/sage-main$ ../../sage -docbuild reference html
sphinx-build -b html -d /mnt/usb1/scratch/novoselt/sage/devel/sage/doc/output/doctrees/en/reference    /mnt/usb1/scratch/novoselt/sage/devel/sage/doc/en/reference /mnt/usb1/scratch/novoselt/sage/devel/sage/doc/output/html/en/reference
Running Sphinx v0.6.3
loading pickled environment... done
building [html]: targets for 798 source files that are out of date
updating environment: [config changed] 802 added, 0 changed, 0 removed
reading sources... [100%] structure
/mnt/usb1/scratch/novoselt/sage/local/lib/python2.6/site-packages/sage/schemes/elliptic_curves/sha_tate.py:docstring of sage.schemes.elliptic_curves.sha_tate.Sha.bound_kato:12: (WARNING/2) Definition list ends without a blank line; unexpected unindent.
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [  0%] coercion
Exception occurred:
  File "/mnt/usb1/scratch/novoselt/sage/devel/sage/doc/common/conf.py", line 444, in find_sage_dangling_links
    fromdocname = getattr(sys.modules[modname], basename).__module__
KeyError: None
The full traceback has been saved in /tmp/sphinx-err-Unu279.log, if you want to report the issue to the author.
Please also report this if it was a user error, so that a better error message can be provided next time.
Send reports to sphinx-dev@googlegroups.com. Thanks!
Build finished.  The built documents can be found in /mnt/usb1/scratch/novoselt/sage/devel/sage/doc/output/html/en/reference
novoselt@sage:/scratch/novoselt/sage/devel/sage-main$

Then I removed this patch, built documentation successfully, pushed this and some other patches and now it seems to work fine. Perhaps it was an unreproducible glitch unrelated to the patch. In general it seems to work as expected and showed me a couple of mistakes in my modules.

I hesitate to switch status to positive review because of the error above and because I don't really understand the code, but I think that this is a great addition and will use this patch from now on!

comment:7 follow-up: Changed 7 years ago by novoselt

  • Status changed from needs_review to needs_work

I tried the same thing on my own computer - upgraded from 4.4.2 to 4.4.3, applied this patch and tried to build documentation (without setting the environment variable this time). Same error repeatedly, but after popping the patch out everything goes OK. So something has to be done ;-)

comment:8 in reply to: ↑ 7 Changed 7 years ago by hivert

  • Status changed from needs_work to needs_review

Hi Andrey

Replying to novoselt:

I tried the same thing on my own computer - upgraded from 4.4.2 to 4.4.3, applied this patch and tried to build documentation (without setting the environment variable this time). Same error repeatedly, but after popping the patch out everything goes OK. So something has to be done ;-)

Thanks for looking at that. My patch was working for .py and .pyx file but not .rst file. The new updated file should work. I didn't get any comment from #sphinx-devel except that missing-reference is the good entry point. So except if some expert pop up and suggest some more improvements, I consider this proposal as a good one. Please review.

comment:9 follow-up: Changed 7 years ago by novoselt

Now I upgraded from 4.4.3 to 4.4.4.alpha0. The new patch works better, but not perfect:

WARNING: undefined symbol :meth:`dual` in module sage.categories.dual
writing output... [ 13%] sage/categories/examples/algebras_with_basis
Exception occurred:
  File "/mnt/usb1/scratch/novoselt/sage/devel/sage/doc/common/conf.py", line 453, in find_sage_dangling_links
    current_module = sys.modules[modname]
KeyError: u'sage.categories.examples.algebras_with_basis'
The full traceback has been saved in /tmp/sphinx-err-rFQQUv.log, if you want to report the issue to the author.
Please also report this if it was a user error, so that a better error message can be provided next time.
Send reports to sphinx-dev@googlegroups.com. Thanks!
Build finished.  The built documents can be found in /mnt/usb1/scratch/novoselt/sage/devel/sage/doc/output/html/en/reference

comment:10 in reply to: ↑ 9 Changed 7 years ago by hivert

Replying to novoselt:

Now I upgraded from 4.4.3 to 4.4.4.alpha0. The new patch works better, but not perfect:

Hum ! I know how to workaround those problems but I don't understand why this is happening ! Unfortunately, I don't want to upgrade and I can't reproduce the bug... Can you try to remove the directory $SAGE_ROOT/devel/sage/doc/output and relaunch the compilation. Does it still bug ?

Florent

comment:11 Changed 7 years ago by novoselt

Successful built with 552 warnings!

comment:12 follow-up: Changed 7 years ago by novoselt

While working on my patches, I am getting the following from this one:

WARNING: undefined symbol :meth:`sage.geometry.fan.RationalPolyhedralFan._compute_cone_lattice` in module sage.geometry.cone 

I don't understand what is wrong. There are no typos and this module does exist in my installation. Is it because I don't import this class/module? Or because it is an underscore method and so there is no record of it in the documentation? (In this case, I actually make a reference to the source code of this method, so I just want the name to be typeset in the same way as other methods, I don't care that the link will not work.)

comment:13 in reply to: ↑ 12 Changed 7 years ago by hivert

Replying to novoselt:

While working on my patches, I am getting the following from this one:

WARNING: undefined symbol :meth:`sage.geometry.fan.RationalPolyhedralFan._compute_cone_lattice` in module sage.geometry.cone 

I don't understand what is wrong. There are no typos and this module does exist in my installation. Is it because I don't import this class/module? Or because it is an underscore method and so there is no record of it in the documentation? (In this case, I actually make a reference to the source code of this method, so I just want the name to be typeset in the same way as other methods, I don't care that the link will not work.)

Hi Andrey,

Thanks for beta testing my patch ! It this module included in the documentation ? More precisely is there a link in some .rst file (probably doc/en/reference/geometry.rst) ? Because if it is not imported nor linked from the doc, Sphinx as no way to find it but importing the module. This is something I tried to avoid for performance reason... If you are still having some problem, can you please make your patch accessible so that I can debug with it. I don't care if the code is not working...

You probably already figured that out, but I must confess that I put this patch here for comment but it is clear that it has not sufficiently been shaken. So many thanks for helping me on that and sorry to cause some trouble.

comment:14 follow-up: Changed 7 years ago by novoselt

Hi Florent!

I have figured out that the problem is with the underscore - if I use a "common" method from the same non-imported patch, link is determined just fine.

It may be a good idea not to give warnings in such cases, but on the other hand it is probably quite rare and there is no need to make logic of this patch more convoluted. (In my case the docstring of one of the functions says "see the source code of ... for more involved examples", since those examples cannot be easily written as standalone code and I didn't want to write something "involved, but artificial.")

Since I really like this functionality, I will continue using your patch (and its fresh versions if they become available) and report new problems, if there will be any. But for the final review we will need someone else, since I don't know how Sphinx works and cannot comment on the code itself.

Thank you! Andrey

comment:15 in reply to: ↑ 14 Changed 7 years ago by hivert

It may be a good idea not to give warnings in such cases, but on the other hand it is probably quite rare and there is no need to make logic of this patch more convoluted. (In my case the docstring of one of the functions says "see the source code of ... for more involved examples", since those examples cannot be easily written as standalone code and I didn't want to write something "involved, but artificial.")

Can you describe more precisely what's happening ? Maybe with a very small example... I'm not sure to understand what you are doing... I have the impression that you are requesting me to remove warnings about private methods (i.e.: starting with underscore).

Since I really like this functionality, I will continue using your patch (and its fresh versions if they become available) and report new problems, if there will be any. But for the final review we will need someone else, since I don't know how Sphinx works and cannot comment on the code itself.

I'll try to ping some on sage-devel.

comment:16 follow-up: Changed 7 years ago by novoselt

I think that I want to have a distinction between "totally wrong names" and names which were successfully found in the Sage library (therefore, it makes sense to reference them), but do not have corresponding entries in the documentation files (therefore, it is not possible to convert them into a working hyperlink). Private/underscore methods are one example (I would like actually to seem them in the documentation "on demand", but maybe there are arguments against it), another is reference to objects in modules which are not included into documentation (maybe there will be no such modules eventually). I hope this is more clear, but in any way it is a small point.

comment:17 in reply to: ↑ 16 Changed 7 years ago by hivert

Replying to novoselt:

I think that I want to have a distinction between "totally wrong names" and names which were successfully found in the Sage library (therefore, it makes sense to reference them), but do not have corresponding entries in the documentation files (therefore, it is not possible to convert them into a working hyperlink). Private/underscore methods are one example (I would like actually to seem them in the documentation "on demand", but maybe there are arguments against it), another is reference to objects in modules which are not included into documentation (maybe there will be no such modules eventually). I hope this is more clear, but in any way it is a small point.

This should be exactly what I'm doing: I issue two different kinds of warning:

  • undefined symbol :%s:`%s` in %s
  • symbol :%s:`%s` linked from %s is defined in %s but not documented

Bu maybe sometime I fail finding a symbol and therefore issue the first warning instead of the second one... Is this happening for you ?

Florent

comment:18 Changed 7 years ago by novoselt

I get the first kind of warning and now the class is actually imported (although in the end of the module to avoid circular imports).

I have just uploaded a fresh patch to #8987 (so fresh, that it is actually very raw ;-)) if you want to reproduce the issue. Beware that it is a chain of patches, I think it is possible to apply only those listed in #9062, #8986, and #8987 (especially if you are working with 4.4.4.alpha0, where all prerequisites but one are merged).

comment:19 follow-up: Changed 7 years ago by novoselt

I have discovered that

:class:`tuple`

in the documentation translates now into a black link

__builtin__.tuple

in the output. I think it would be better to display links exactly as they were written originally and "expand" only the actual link, if it is working.

comment:20 in reply to: ↑ 19 ; follow-up: Changed 7 years ago by hivert

Replying to novoselt:

I have discovered that

:class:`tuple`

in the documentation translates now into a black link

__builtin__.tuple

in the output. I think it would be better to display links exactly as they were written originally and "expand" only the actual link, if it is working.

Yes ! That was before I know how to resolve those with intersphinx. See my new patches

comment:21 Changed 7 years ago by hivert

  • Description modified (diff)

comment:22 in reply to: ↑ 20 ; follow-up: Changed 7 years ago by hivert

  • Status changed from needs_review to needs_work

Replying to hivert:

Yes ! That was before I know how to resolve those with intersphinx. See my new patches

There is still a problem: currently when linking to python, I raise a warning before intersphinx tries to find the link in Python's database.

comment:23 in reply to: ↑ 22 Changed 7 years ago by hivert

  • Work issues set to Upgrade to Sphinx 1.0

There is still a problem: currently when linking to python, I raise a warning before intersphinx tries to find the link in Python's database.

Some update: It is not possible to achieve what I want in Sphinx 0.6 without hacking into sphinx. However Sphinx 1.0 should be out very soon (they released 1.0beta2 last week). At raising warning is builtin in sphinx 1.0 with the correct configuration option. So I'll probably wait until sphinx 1.0 is out to finish this one, unless someone insist to have it very soon.

comment:24 Changed 7 years ago by novoselt

My opinion is that upgrading Sphinx is the way to go, it may have other benefits (and hopefully no drawbacks). I really want this functionality but I am quite happy with the availability of these patches here, thank you for writing them, Florent!

Andrey

comment:25 Changed 7 years ago by hivert

For the record, here is another bad side effect of this patch:

sage: PolynomialRing?

Type:		function
Base Class:	<type 'function'>
String Form:	<function PolynomialRing at 0x16a2758>
Namespace:	Interactive
File:		/usr/devel/sage/sage/rings/polynomial/polynomial_ring_constructor.py
Definition:	PolynomialRing(base_ring, arg1=None, arg2=None, sparse=False, order='degrevlex', names=None, name=None, implementation=None)
Docstring:
    <no docstring>

comment:26 Changed 7 years ago by novoselt

Is it possible to display line numbers in warnings about bad links?

comment:27 Changed 7 years ago by novoselt

I guess it was expected: these patches do not work anymore in Sage-4.6.1.alpha3 due to Sphinx upgrade. On a fresh installation I got

novoselt@sage:/scratch/novoselt/sage/devel/sage-main$ hg qapplied
novoselt@sage:/scratch/novoselt/sage/devel/sage-main$ hg qpush
applying trac_9128-sphinx_links_all-fh.patch
now at: trac_9128-sphinx_links_all-fh.patch
novoselt@sage:/scratch/novoselt/sage/devel/sage-main$ hg qpush
applying trac_9128-intersphinx_python_database-fh.patch
now at: trac_9128-intersphinx_python_database-fh.patch
novoselt@sage:/scratch/novoselt/sage/devel/sage-main$ ../../sage -b
...
novoselt@sage:/scratch/novoselt/sage/devel/sage-main$ ../../sage -docbuild reference html
sphinx-build -b html -d /mnt/usb1/scratch/novoselt/sage/devel/sage/doc/output/doctrees/en/reference    /mnt/usb1/scratch/novoselt/sage/devel/sage/doc/en/reference /mnt/usb1/scratch/novoselt/sage/devel/sage/doc/output/html/en/reference
Running Sphinx v1.0.4
loading pickled environment... done
loading intersphinx inventory from /mnt/usb1/scratch/novoselt/sage/devel/sage/doc/common/python-inv.txt...
building [html]: targets for 1 source files that are out of date
updating environment: [extensions changed] 882 added, 0 changed, 0 removed
reading sources... [100%] tensor                                                                                                                              
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [  0%] coercion                                                                                                                             
Exception occurred:
  File "/mnt/usb1/scratch/novoselt/sage/devel/sage/doc/common/conf.py", line 446, in find_sage_dangling_links
    modname = nodeattr['modname']
KeyError: 'modname'
The full traceback has been saved in /tmp/sphinx-err-5Yrzg5.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
Either send bugs to the mailing list at <http://groups.google.com/group/sphinx-dev/>,
or report them in the tracker at <http://bitbucket.org/birkenfeld/sphinx/issues/>. Thanks!
Build finished.  The built documents can be found in /mnt/usb1/scratch/novoselt/sage/devel/sage/doc/output/html/en/reference

comment:28 Changed 6 years ago by hivert

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Work issues Upgrade to Sphinx 1.0 deleted

I upgraded my patch to the new sphinx. I'm not sure the patch is completely finished but I'd be very interesting to have feedback.

comment:29 Changed 6 years ago by hivert

  • Description modified (diff)

comment:30 follow-up: Changed 6 years ago by novoselt

Hi Florent,

I have enjoyed using your patch in the past and I am going to start using it again but so far it does not apply to sage-4.7.rc0:

novoselt@tx2-LM:~/sage/devel/sage-main$ hg qimport http://trac.sagemath.org/sage_trac/raw-attachment/ticket/9128/trac_9128-intersphinx_python_database-fh.patch
adding trac_9128-intersphinx_python_database-fh.patch to series file
novoselt@tx2-LM:~/sage/devel/sage-main$ hg qpush
applying trac_9128-intersphinx_python_database-fh.patch
now at: trac_9128-intersphinx_python_database-fh.patch
novoselt@tx2-LM:~/sage/devel/sage-main$ hg qimport http://trac.sagemath.org/sage_trac/raw-attachment/ticket/9128/trac_9128-sphinx_links_all-fh.patch
adding trac_9128-sphinx_links_all-fh.patch to series file
novoselt@tx2-LM:~/sage/devel/sage-main$ hg qpush
applying trac_9128-sphinx_links_all-fh.patch
patching file doc/common/conf.py
Hunk #2 FAILED at 19
Hunk #3 succeeded at 97 with fuzz 2 (offset -7 lines).
1 out of 6 hunks FAILED -- saving rejects to file doc/common/conf.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh trac_9128-sphinx_links_all-fh.patch
novoselt@tx2-LM:~/sage/devel/sage-main$ 

This is on a just built installation without any other patches.

comment:31 in reply to: ↑ 30 Changed 6 years ago by hivert

  • Description modified (diff)

Hi Andrey,

Replying to novoselt:

I have enjoyed using your patch in the past and I am going to start using it again but so far it does not apply to sage-4.7.rc0:

Oops !!! I should have said that this depend on #11251. Thanks for pointing it out and for testing my patch.

comment:32 Changed 6 years ago by hivert

I updated a little my patch to make sure that the reference guide is compiled first. This is needed so that other documentation can link to the reference guide. Please review.

comment:33 Changed 6 years ago by jdemeyer

  • Dependencies set to #11251
  • Description modified (diff)

comment:34 Changed 6 years ago by hivert

  • Cc mhansen added

Hi Mike,

I just added you in CC remembering that you are the Sphinx expert. If you can be kind enough to give me some feedback on that one, I'd very appreciate. It was a very though one for me as sphinx doesn't expose the necessary interface. So I had to dig in the internal. I think it is very useful and it could greatly help improving the documentation.

Thanks for your help.

comment:35 follow-up: Changed 6 years ago by jhpalmieri

By the way, note that #6495 also touches some of the same files, and for builder.py, in a conflicting way. I think that #6495 could be rebased on top of this one.

comment:36 in reply to: ↑ 35 Changed 6 years ago by jhpalmieri

Replying to jhpalmieri:

By the way, note that #6495 also touches some of the same files, and for builder.py, in a conflicting way. I think that #6495 could be rebased on top of this one.

(Although I'm not sure, since #6495 completely changes how the reference manual is built. Can you take a look at that one to see what you think and whether the two approaches can be combined?)

comment:37 follow-up: Changed 6 years ago by novoselt

Patches do not apply anymore on top of Sage-5.0.beta4. (Perhaps because of recent :trac: addition in another ticket?) What are actually the plans for this ticket?..

comment:38 in reply to: ↑ 37 Changed 6 years ago by hivert

Replying to novoselt:

Patches do not apply anymore on top of Sage-5.0.beta4. (Perhaps because of recent :trac: addition in another ticket?) What are actually the plans for this ticket?..

Yes there is a conflict with :trac:. I have a rebased patch in the sage-combinat queue which should applies. However I didn't test if it still works. I check it tomorrow and repost it. I'll be more that happy to have a review for this one. It has been a huge pain to get to a working solution, so I'll end up very frustrated if it goes to the garbage.

Florent

comment:39 Changed 6 years ago by hivert

  • Dependencies changed from #11251 to #11251, #12490
  • Description modified (diff)

Hi there,

I'm uploading two new patches:

  • trac_9128-intersphinx_python_database-fh.patch updated to Python 2.7

  • trac_9128-sphinx_links_all-fh.patch rebased for Sage-5.0.alpha4 and in particular on top of #12490 which was previously in this ticket.

As I already said, I'm not sure if this should enter Sage in the present status but I need a Sphinx expert (if not George Brandl himself) to try to polish and simplify my code. However, I think the feature is quite important in order to improve Sage's doc, so I suggest that if no expert jump up to let the code enter Sage as such and to improve it in a former ticket. I therefore leave it as need-review.

comment:40 follow-up: Changed 6 years ago by nthiery

+1 on getting it merged in 5.0, that is as soon as possible! As it is, it already helps much improving the Sage documentation, and it can be improved later. Besides, we want to use it for working with the Sage-Combinat patches, but it's a pain to have it in the queue since it forces recompiling all the documentation.

I have been through the patch, and it looks reasonnable, though I am not by far a Sphinx expert. Just a detail: several of the functions do not have doctests. Now I am not sure if it is anyway really possible to doctest them; if not, I guess that's ok as is.

Andrei or John: would you agree to put a positive review?

Cheers,

Nicolas

comment:41 Changed 6 years ago by novoselt

I never tried to figure out how Sphinx works, so I cannot say much about the code itself. However, I have been following this ticket from the very beginning, having it on the top of my queue whenever it was cleanly applicable. I have never had any issues with it and it helped me to catch some dangling links. I didn't try to use it for short links in documentation, since I didn't want to be dependent on this ticket, but I surely do want to avoid figuring out and typing every full path. This also makes the documentation resistant to refactoring and moving functions around.

So - as a user I like this patch a lot, give it positive review, and vote for inclusion as is!

comment:42 in reply to: ↑ 40 Changed 6 years ago by hivert

I have been through the patch, and it looks reasonnable, though I am not by far a Sphinx expert. Just a detail: several of the functions do not have doctests. Now I am not sure if it is anyway really possible to doctest them; if not, I guess that's ok as is.

Unfortunately, I've no idea how to doctests those. As you can see from the code, I wrote my patch using log backtrace and pdb. At several point, I'm using call to sphinx or docutils internal which are not really documented. So this is some kind of reverse engineering. I tried several time to ask for some help on sphinx-user and never got any answer on that. My diagnostic is that Sphinx doesn't expose a sufficiently flexible API to achieve what we want.

Probably a good solution would be to have a Sage-days whose subject is Sphinx and doc-compiling and invite George Brandl. We could also solve the Sphinx parallel build ticket at the same occasion. Unfortunately, I just organized a Sage-days and I'm invited to two other until summer so I won't organize such a days and if organized, I don't think I'll be able to attend.

By the way, I'll CC this on Sage-devel.

comment:43 Changed 6 years ago by hivert

I just added the following diff which should resolve many more dependance to python itself.

  • doc/common/conf.py

    diff --git a/doc/common/conf.py b/doc/common/conf.py
    a b def call_intersphinx(app, env, node, con 
    490490        debug_inf(app, "---- Intersphinx: %s not Found"%node['reftarget'])
    491491    return res
    492492
     493# lists of basic Python class which are documented as functions
     494base_class_as_func = [
     495    'bool', 'complex', 'dict', 'file', 'float',
     496    'frozenset', 'int', 'list', 'long', 'object',
     497    'set', 'slice', 'str', 'tuple', 'type', 'unicode', 'xrange']
    493498
    494499def find_sage_dangling_links(app, env, node, contnode):
    495500    """
    def find_sage_dangling_links(app, env, n 
    507512
    508513    debug_inf(app, "Searching %s from %s"%(reftarget, doc))
    509514
    510     # Workaround: in Python's doc 'object' is documented as a function rather
    511     # than a class
    512     if reftarget == 'object' and reftype == 'class':
     515    # Workaround: in Python's doc 'object', 'list', ... are documented as a
     516    # function rather than a class
     517    if reftarget in base_class_as_func and reftype == 'class':
    513518        node['reftype'] = 'func'
    514519
    515520    res = call_intersphinx(app, env, node, contnode)

comment:44 Changed 6 years ago by hivert

The way we called Sphinx, it wasn't honoring the SPHINXOPTS environment variable anymore. As a consequence option -n wasn't working anymore. I just uploaded a patch which fixes this. Maybe this should be added to the dev-guide.

comment:45 Changed 6 years ago by nthiery

  • Reviewers set to Andrey Novoseltsev, Nicolas M. Thiéry

Ok, unless someone comments before that, I'll set a positive review tomorrow morning.

comment:46 Changed 6 years ago by nthiery

  • Status changed from needs_review to positive_review

comment:47 Changed 6 years ago by hivert

  • Status changed from positive_review to needs_work

There is a little problem in the handling of the environment + Usefull sphinx options should be better documented. I need to rework this a little.

Florent

Changed 6 years ago by hivert

comment:48 Changed 6 years ago by hivert

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Hi there,

I finalized the doc of this patch. I also took the chance to add a extra option 'warn-links' which make Sphinx complains for dangling links. To ease the review, I uploaded my changes in trac_9128-doc_option-fh.patch. Those changes are folded in trac_9128-sphinx_links_all-fh.patch so that you only need to apply this one and the database patch.

Changed 6 years ago by nthiery

comment:49 Changed 6 years ago by nthiery

  • Status changed from needs_review to positive_review

I made two last minor changes to Florent's patch, with his agreement, and reposted it. Good to go!

comment:50 Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Unfortunately, this breaks the pdf reference manual:

! TeX capacity exceeded, sorry [main memory size=1500000].
<argument> ...\endcsname \current@color {0.40,0.40
                                                  ,0.40}\set@color
l.47055 ...1}\PYG{o}{/}\PYG{l+m+mf}{0.1}\PYG{p}{)}

!  ==> Fatal error occurred, no output PDF file produced!
Transcript written on reference.log.
make[1]: *** [reference.pdf] Error 1
make[1]: Leaving directory `/mnt/usb1/scratch/jdemeyer/merger/sage-5.0.beta5-9128/devel/sage-main/doc/output/latex/en/reference'
Build finished.  The built documents can be found in /mnt/usb1/scratch/jdemeyer/merger/sage-5.0.beta5-9128/devel/sage/doc/output/pdf/en/reference

This is using

$ latex --version
pdfTeX using libpoppler 3.141592-1.40.3-2.2 (Web2C 7.5.6)
kpathsea version 3.5.6
Copyright 2007 Peter Breitenlohner (eTeX)/Han The Thanh (pdfTeX).
Kpathsea is copyright 2007 Karl Berry and Olaf Weber.
There is NO warranty.  Redistribution of this software is
covered by the terms of both the pdfTeX using libpoppler copyright and
the Lesser GNU General Public License.
For more information about these matters, see the file
named COPYING and the pdfTeX using libpoppler source.
Primary author of pdfTeX using libpoppler: Peter Breitenlohner (eTeX)/Han The Thanh (pdfTeX).
Kpathsea written by Karl Berry, Olaf Weber, and others.

Compiled with libpng 1.2.15beta5; using libpng 1.2.15beta5
Compiled with zlib 1.2.3.3; using zlib 1.2.3.3
Compiled with libpoppler version 3.00

From Googling around, this might not even be fixable without recompiling LaTeX or at least changing LaTeX's configuration files.

comment:51 follow-up: Changed 6 years ago by jdemeyer

Let me clarify that this happened on sage.math, where the memory size is set to 1500000. On a different system with 3000000 as limit, it compiled. The end of that pdflatex log file shows:

Here is how much of TeX's memory you used:
 59038 strings out of 494632
 2184818 string characters out of 3923881
 1820512 words of memory out of 3000000
 34726 multiletter control sequences out of 10000+50000
 99604 words of font info for 164 fonts, out of 3000000 for 5000
 298 hyphenation exceptions out of 8191
 43i,25n,51p,6802b,946s stack positions out of 5000i,500n,10000p,200000b,50000s

I don't really see a solution besides requiring people to change their LaTeX installs, or splitting up the reference manual (would #6495 fix this?)

comment:52 Changed 6 years ago by jhpalmieri

#6495 might fix this, since it breaks the reference manual up into smaller pieces. The only question is whether the pieces are small enough...

comment:53 in reply to: ↑ 51 ; follow-up: Changed 6 years ago by hivert

Hi,

Replying to jdemeyer:

Let me clarify that this happened on sage.math, where the memory size is set to 1500000. On a different system with 3000000 as limit, it compiled. The end of that pdflatex log file shows:

\begin{GROUMPH} First of all, I need to express my feelings: Fucking Sphinx, Fucking Latex. None of these two software where seriously designed to scale to a project of Sage size. That's a shame.

While I'm at it, fucking Sagemath distro. It worked without any problem on my laptop. \end{GROUMPH}

Sorry for this non useful comment.

I'm quite angry and frustrated because what my patch does is only to fix a huge bunch of missing links in Sage doc way before they are sent to the docutil writer for HTML or TeX. My patch is working at the level of docutil abstract syntax tree, but I could have fixed those link by editing Sage source as well. This means that if the doc of Sage was correct, it won't compile either ! And now because I tried to fix the doc, I'm asked to fix LaTeX. I don't think it is really fair.

I don't really see a solution besides requiring people to change their LaTeX installs, or splitting up the reference manual (would #6495 fix this?)

This also means that, whether or not we apply this patch, we will hit the same problem again, as the doc of Sage is expected to grow.


Let's try to be more constructive. I see several solution:

  • add a comment in Sage installation instructions saying that TeX limitation

should be enhanced to compile the PDF doc (by the way, are there any people really using the PDF doc out there ?)

  • is it really a hard limitation. Couldn't this be fixed by a shell variable ?

I had the following script in my ~/bin when I was using XY-pic:

popcorn-*binat/doc/common $ cat ~/bin/hugetex
#!/bin/sh
#####################################
# Boosted TeX to compile withc XY-pic
export extra_mem_bot=8000000; tex $*

Jeroen: will you be so kind to try if this works ?

  • I can maybe disable my link fixing code when we are compiling to PDF, but I

think this is really a temporary bugware solution. Considering the time I already lost on this ticket and the fact that this doesn't solve the problem but barely hide it, I don't think this is an acceptable solution.

Florent

comment:54 follow-up: Changed 6 years ago by hivert

Hi,

Replying to jdemeyer:

Let me clarify that this happened on sage.math, where the memory size is set to 1500000. On a different system with 3000000 as limit, it compiled. The end of that pdflatex log file shows:

\begin{GROUMPH}

First of all, I need to express my feelings: Fucking Sphinx, Fucking Latex. None of these two software where seriously designed to scale to a project of Sage's size. That's a shame.

While I'm at it, fucking Sagemath distro. It worked without any problem on my laptop.

\end{GROUMPH}

Sorry for this non useful comment.

I'm quite angry and frustrated because what my patch does is only to fix a huge bunch of missing links in Sage doc, way before they are sent to the docutil writer for HTML or TeX. My patch is working at the level of docutil abstract syntax tree, but I could have fixed those link by editing Sage source as well. This means that if the doc of Sage was correct, it wouldn't compile either ! And now because I tried to fix the doc, I'm asked to fix LaTeX. I don't think it is really fair.

I don't really see a solution besides requiring people to change their LaTeX installs, or splitting up the reference manual (would #6495 fix this?)

This also means that, whether or not we apply this patch, we will hit the same problem again, as the doc of Sage is expected to grow.


Let's try to be more constructive. I see several solutions:

  • add a comment in Sage installation instructions saying that TeX limitation

should be enhanced to compile the PDF doc (by the way, are there any people really using the PDF doc out there ?)

  • is it really a hard limitation ? Couldn't this be fixed by a shell variable ?

I had the following script in my ~/bin when I was using XY-pic:

#!/bin/sh
#####################################
# Boosted TeX to compile withc XY-pic
export extra_mem_bot=8000000; tex $*

Jeroen: will you be so kind to try if this works ?

  • I can maybe disable my link fixing code when we are compiling to PDF, but I

think this is really a temporary bugware solution. Considering the time I already lost on this ticket and the fact that this doesn't solve the problem but barely hide it under the carpet, I don't think this is an acceptable solution.

What do you think ?

Florent

comment:55 Changed 6 years ago by hivert

Sorry for the double answer. Please only look at the second one.

comment:56 in reply to: ↑ 53 Changed 6 years ago by jdemeyer

Replying to hivert:

First of all, I need to express my feelings: Fucking Sphinx, Fucking Latex.

I never understood why Donald Knuth, who has written so much about algorithms, apparently doesn't know about dynamic memory allocation...

comment:57 in reply to: ↑ 54 Changed 6 years ago by jdemeyer

Replying to hivert:

export extra_mem_bot=8000000; tex $*

Jeroen: will you be so kind to try if this works ?

This sort of works. It builds the manual, but here's the strange thing: every run of pdflatex consumes more and more memory. That is, if I run pdflatex 10 times in a row on reference.tex with that extra environment variable, I will certainly hit the error again.

comment:58 Changed 6 years ago by jdemeyer

  • Dependencies changed from #11251, #12490 to #11251, #12490, #12572
  • Status changed from needs_work to positive_review

With extra_mem_top=2000000 (note bot vs. top), it seems to work as it should:

Here is how much of TeX's memory you used:
 57776 strings out of 94101
 2165001 string characters out of 3915810
 1541607 words of memory out of 2966904
 33568 multiletter control sequences out of 10000+50000
 99604 words of font info for 164 fonts, out of 1200000 for 2000
 638 hyphenation exceptions out of 8191
 38i,25n,51p,6802b,946s stack positions out of 5000i,500n,6000p,200000b,50000s

See #12572.

comment:59 Changed 6 years ago by jdemeyer

The new Sphinx spkg to add extra memory to latex is ready for review at #12572.

comment:60 follow-up: Changed 6 years ago by jdemeyer

The files

doc/common/python.inv
doc/common/update-python-inv.sh

should be put in SAGE_ROOT/devel/sage/MANIFEST.in

comment:61 Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_work
  • Work issues set to MANIFEST.in

Changed 6 years ago by hivert

comment:62 in reply to: ↑ 60 Changed 6 years ago by hivert

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Replying to jdemeyer:

The files

doc/common/python.inv
doc/common/update-python-inv.sh

should be put in SAGE_ROOT/devel/sage/MANIFEST.in

Thanks for pointing this out. I just added a patch trac_9128-MANIFEST-fh.patch which should do that. Please double check as I don't really know what should be there.

Florent

comment:63 Changed 6 years ago by nthiery

  • Status changed from needs_review to positive_review

This looks good. Back to positive review!

comment:64 Changed 6 years ago by jdemeyer

  • Work issues MANIFEST.in deleted

comment:65 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.0.beta8
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:66 Changed 6 years ago by nthiery

Yippee, a good thing done!

Thanks Florent for all the energy you put into this ticket :-) Thanks everyone for the review and support.

comment:67 Changed 5 years ago by jdemeyer

See #12849 for a blocker follow-up: The argspecs of extension function/methods is broken in the Sphinx documentation.

comment:68 Changed 5 years ago by bober

See #13057 for a speed regression followup. It seems that this ticket slowed down introspection quite a bit.

comment:69 Changed 5 years ago by kini

This ticket also introduced a memory leak - 56 MB per docstring lookup. See #13057 and sage-devel.

Note: See TracTickets for help on using tickets.