Ticket #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 | Work issues: | |
| Report Upstream: | N/A | Reviewers: | Andrey Novoseltsev, Nicolas M. Thiéry |
| Authors: | Florent Hivert | Merged in: | sage-5.0.beta8 |
| Dependencies: | #11251, #12490, #12572 | Stopgaps: |
Description (last modified by hivert) (diff)
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
Change History
comment:4 Changed 3 years ago by hivert
- Status changed from needs_work to needs_review
- Description modified (diff)
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:6 Changed 3 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: ↓ 8 Changed 3 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 3 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: ↓ 10 Changed 3 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 3 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 3 years ago by novoselt
Successful built with 552 warnings!
comment:12 follow-up: ↓ 13 Changed 3 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 3 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.coneI 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: ↓ 15 Changed 3 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 3 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: ↓ 17 Changed 3 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 3 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 3 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: ↓ 20 Changed 3 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: ↓ 22 Changed 3 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:22 in reply to: ↑ 20 ; follow-up: ↓ 23 Changed 3 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 3 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 3 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 3 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 3 years ago by novoselt
Is it possible to display line numbers in warnings about bad links?
comment:27 Changed 2 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 2 years ago by hivert
- Status changed from needs_work to needs_review
- Description modified (diff)
- 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:30 follow-up: ↓ 31 Changed 2 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 2 years ago by hivert
- Description modified (diff)
comment:32 Changed 2 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:34 Changed 2 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: ↓ 36 Changed 20 months ago by jhpalmieri
comment:36 in reply to: ↑ 35 Changed 20 months 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: ↓ 38 Changed 15 months 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 15 months 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 15 months 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: ↓ 42 Changed 15 months 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 15 months 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 15 months 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 15 months 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 490 490 debug_inf(app, "---- Intersphinx: %s not Found"%node['reftarget']) 491 491 return res 492 492 493 # lists of basic Python class which are documented as functions 494 base_class_as_func = [ 495 'bool', 'complex', 'dict', 'file', 'float', 496 'frozenset', 'int', 'list', 'long', 'object', 497 'set', 'slice', 'str', 'tuple', 'type', 'unicode', 'xrange'] 493 498 494 499 def find_sage_dangling_links(app, env, node, contnode): 495 500 """ … … def find_sage_dangling_links(app, env, n 507 512 508 513 debug_inf(app, "Searching %s from %s"%(reftarget, doc)) 509 514 510 # Workaround: in Python's doc 'object' is documented as a function rather511 # than a class512 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': 513 518 node['reftype'] = 'func' 514 519 515 520 res = call_intersphinx(app, env, node, contnode)
comment:44 Changed 15 months 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 15 months 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:47 Changed 15 months 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
comment:48 Changed 15 months ago by hivert
- Status changed from needs_work to needs_review
- Description modified (diff)
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.
comment:49 Changed 15 months 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 15 months 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: ↓ 53 Changed 15 months 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 15 months 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: ↓ 56 Changed 15 months 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: ↓ 57 Changed 15 months 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 15 months ago by hivert
Sorry for the double answer. Please only look at the second one.
comment:56 in reply to: ↑ 53 Changed 15 months 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 15 months 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 15 months ago by jdemeyer
- Status changed from needs_work to positive_review
- Dependencies changed from #11251, #12490 to #11251, #12490, #12572
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 15 months ago by jdemeyer
The new Sphinx spkg to add extra memory to latex is ready for review at #12572.
comment:60 follow-up: ↓ 62 Changed 15 months 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 15 months ago by jdemeyer
- Status changed from positive_review to needs_work
- Work issues set to MANIFEST.in
comment:62 in reply to: ↑ 60 Changed 15 months ago by hivert
- Status changed from needs_work to needs_review
- Description modified (diff)
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 15 months ago by nthiery
- Status changed from needs_review to positive_review
This looks good. Back to positive review!
comment:65 Changed 15 months ago by jdemeyer
- Status changed from positive_review to closed
- Resolution set to fixed
- Merged in set to sage-5.0.beta8
comment:66 Changed 15 months 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 13 months 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 12 months ago by bober
See #13057 for a speed regression followup. It seems that this ticket slowed down introspection quite a bit.
comment:69 Changed 12 months ago by kini
This ticket also introduced a memory leak - 56 MB per docstring lookup. See #13057 and sage-devel.


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