Opened 18 months ago

Closed 13 months ago

Last modified 10 months ago

#25382 closed defect (fixed)

py3: do not include the notebook documentation in sage

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.5
Component: notebook Keywords:
Cc: embray, jdemeyer, tscrim, fbissey, vbraun, kcrisman, jhpalmieri Merged in:
Authors: John Palmieri Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 591e91a (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by chapoton)

because sagenb is not yet available for python3

Change History (73)

comment:1 Changed 18 months ago by kcrisman

  • Cc kcrisman added

comment:2 Changed 18 months ago by chapoton

  • Branch set to public/25382
  • Commit set to 501b4d20341a54646ab4c806942bd0ea74e1bc22
  • Status changed from new to needs_info

I have no clear idea about what should be done here. Where will the sagenb doc live, if we get rid of it ?


New commits:

501b4d2trying to remove sagenb doc

comment:3 Changed 18 months ago by embray

Are we never going to get sagenb working in Python 3? I've just been assuming that for now we don't have it, but that we would eventually port it at least for now. Or are we truly abandoning sagenb for Python 3?

comment:4 Changed 18 months ago by chapoton

Well, if somebody wants to do the job of converting sagenb to full python3 compatibility.. I have no idea of the difficulty of the task.

comment:5 Changed 18 months ago by vbraun

I'm fine with removing sagenb when we drop Python2 support unless somebody wants to port it... The SageNB docs should just say that (and suggest to use the jupyter notebook)

comment:6 Changed 18 months ago by kcrisman

If the ETA is still some time from now for Py2 drop that seems plausible. But hopefully it won't be that bad; I feel like #22431 was closer than we thought.

comment:7 Changed 18 months ago by chapoton

We should try in the ticket #22431 to see if we can re-activate sagenb in py3-sage. We decided some time ago to de-activate it as it was blocking the build of py3-sage. Try to-re-insert sagenb requires first #24269 to fix py3-sage. Then rewrite the spkg-install on top of #25394.

On the side-point to get rid of sagenb imports inside sage, tt would be helpful to positive-review #24994.

comment:8 Changed 18 months ago by chapoton

  • Milestone changed from sage-8.3 to sage-duplicate/invalid/wontfix
  • Status changed from needs_info to needs_review

let us close this one as invalid, as we are on the way towards building the doc on py3, including sagenb doc

comment:9 Changed 18 months ago by gh-timokau

I think this is still a good idea (for python2 and python3). Given that sagenb has been deprecated for a while, I think it would be a good idea to either just remove it or make it a optional spkg. #24994 is a nice step in that direction and this would be another. Why should the documentation of the deprecated sagenb be included in sage when the documentation of other spkgs isn't?

For context: sagenb is a pain to package. First because of https://github.com/sagemath/sagenb/issues/440 and now while updating flask I encountered more problems.

comment:10 Changed 17 months ago by kcrisman

Deprecated in what sense? Unofficially, sure - but probably this would need to be a much bigger announcement and very obvious that it is removed, rather than the (very sensible) procedure of upgrade option we currently offer.

comment:11 Changed 17 months ago by vbraun

+1 to communicating the deprecation better, e.g. display a deprecation notice when you start sagenb. I don't really mind it being standard vs. optional, I just don't want somebody new to Sage get started with sagenb in 2018 just because it comes up by default.

comment:12 Changed 17 months ago by gh-timokau

Deprecated because I was told so in https://github.com/sagemath/sagenb/issues/440 (I was surprised there so I agree that it should be communicated better). Also the docs refer to it as the "legacy SageNB".

comment:13 follow-ups: Changed 17 months ago by kcrisman

I don't think that anyone would be having it come up by default, the default is Jeroen's converter and pretty soon most people's default ends up the Jupyter. Maybe an intermediate thing to being an optional package (which is very bad for the kind of end users who would need sagenb!) would be to formally change the default interface to being Jupyter, with VERY clear instructions for how to continue conversion after that point sprinkled everywhere.

comment:14 in reply to: ↑ 13 Changed 17 months ago by jdemeyer

Replying to kcrisman:

Jeroen's converter

Just to give proper credit: Volker started that project and did most of the work. It's true that I also worked on it in order to make it the default interface, fixing various bugs and adding the "run SageNB" option.

comment:15 in reply to: ↑ 13 Changed 17 months ago by gh-timokau

Replying to kcrisman:

I don't think that anyone would be having it come up by default, the default is Jeroen's converter and pretty soon most people's default ends up the Jupyter.

I think that is even more of a reason to work towards making sagenb optional.

Maybe an intermediate thing to being an optional package (which is very bad for the kind of end users who would need sagenb!)

Are we sure there even are users that keep up with the latest version of sage (e.g. aren't using 6.0 forever or something) *and* still use sagenb? And even making sagenb optional wouldn't immediately make that impossible, it would just make it a tiny bit more inconvenient. That might be a good thing because then they might either complain here (showing us that there are such users) or realize that it might be time to look into the jupyter notebook.

would be to formally change the default interface to being Jupyter, with VERY clear instructions for how to continue conversion after that point sprinkled everywhere.

I agree.

comment:16 follow-up: Changed 17 months ago by fbissey

<clear throat>Well in the math department at my university (university of Canterbury, New Zealand) they have reasonably recent installs of sage. But when I talk to them they also have hundreds of sagenb notebook because they have been using sage for quite some time. What they are after is literally a mass conversion tool. And even with one, the inertia of the old sagenb here sounds quite big.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 17 months ago by gh-timokau

Ah, good to know. What about that converter kcrisman was talking about?

comment:18 in reply to: ↑ 17 ; follow-up: Changed 17 months ago by fbissey

Replying to gh-timokau:

Ah, good to know. What about that converter kcrisman was talking about?

There is probably a way to script it but it's just no one really want to take that job. I may have to do it for them at some point but that will be a tough sell.

Last edited 17 months ago by fbissey (previous) (diff)

comment:19 in reply to: ↑ 18 Changed 17 months ago by gh-timokau

Replying to fbissey:

Replying to gh-timokau:

Ah, good to know. What about that converter kcrisman was talking about?

There is probably a way to script it but it's just no one really want to take that job. I may have to do it for them at some point but that will be a tough sell.

Yeah I get that. We could still make the deprecation more obvious and further disentangle sagenb and sage. Maybe a big deprecation warning in 8.3 and then making it optional in 8.4 or something.

comment:20 Changed 17 months ago by chapoton

  • Component changed from python3 to notebook
  • Description modified (diff)

comment:21 Changed 16 months ago by chapoton

  • Milestone changed from sage-duplicate/invalid/wontfix to sage-8.4
  • Status changed from needs_review to needs_info

Removing sagenb doc would be one step towards building the doc of sage with python3. Otherwise, one needs to fix https://github.com/sagemath/sagenb/issues/454.

comment:22 Changed 16 months ago by git

  • Commit changed from 501b4d20341a54646ab4c806942bd0ea74e1bc22 to 820100e10f17de34633447e8c4a7d235c0185dc0

Branch pushed to git repo; I updated commit sha1. New commits:

affbb9eMerge branch 'public/25382' in 8.4.b0
820100eproposal : refer to github sagenb sources

comment:23 Changed 16 months ago by chapoton

  • Cc jhpalmieri added

any comment on the proposed removal ?

comment:24 Changed 16 months ago by jhpalmieri

Is there a way to conditionally remove the docs, depending on whether Sage was built with Python 2 or 3?

comment:25 Changed 16 months ago by kcrisman

Agreed.

comment:26 Changed 16 months ago by jhpalmieri

By the way, with the branch here, make doc and make doc-pdf still try to build the notebook directory in src/doc/en/reference. So for this to work with Python 3, as long as SageNB is not Python 3 compatible, we have to rewrite /src/doc/en/reference/notebook/index.rst as well. It may not matter what goes into that file when using Python 3, since it won't be referenced from anywhere.

comment:27 Changed 16 months ago by jhpalmieri

My idea was something like this:

  • src/doc/en/reference/conf.py

    diff --git a/src/doc/en/reference/conf.py b/src/doc/en/reference/conf.py
    index 9e55717545..9b5cf0559b 100644
    a b import sys, os 
    1515from sage.env import SAGE_DOC_SRC, SAGE_DOC
    1616sys.path.append(SAGE_DOC_SRC)
    1717from common.conf import *
     18from sage_setup.docbuild.build_options import OMIT
    1819
    1920ref_src = os.path.join(SAGE_DOC_SRC, 'en', 'reference')
    2021ref_out = os.path.join(SAGE_DOC, 'html', 'en', 'reference')
    multidocs_is_master = True 
    6162# for 'static' and 'templates', and to deal with upgrades: 'sage',
    6263# 'sagenb', 'media', and 'other'.
    6364bad_directories = ['static', 'templates', 'sage', 'sagenb', 'media', 'other']
     65for dir in OMIT:
     66    if dir.startswith('reference' + os.sep):
     67        bad_directories.append(dir.split(os.sep)[1])
    6468multidocs_subdoc_list = sorted([x for x in os.listdir(ref_src)
    6569                                if os.path.isdir(os.path.join(ref_src, x))
    6670                                and x not in bad_directories])
  • src/doc/en/reference/index.rst

    diff --git a/src/doc/en/reference/index.rst b/src/doc/en/reference/index.rst
    index 640df94c4a..91e6437045 100644
    a b available below. 
    1212User Interface
    1313==============
    1414
    15 * :doc:`Command Line Interface (REPL) <repl/index>`
     15.. only:: Python2
    1616
    17 * For the jupyter notebook interface, see this `Documentation <https://jupyter-notebook.readthedocs.io/en/latest/notebook.html>`_.
     17    * :doc:`Command Line Interface (REPL) <repl/index>`
     18    * :doc:`Web Notebook <notebook/index>`
    1819
    19 * For the legacy notebook interface, which is no longer actively maintained, see the `source repository <https://github.com/sagemath/sagenb>`_.
     20.. only:: Python3
     21
     22    * :doc:`Command Line Interface (REPL) <repl/index>`
     23    * For the jupyter notebook interface, see this `Documentation <https://jupyter-notebook.readthedocs.io/en/latest/notebook.html>`_.
     24    * For the legacy notebook interface, which is no longer actively maintained, see the `source repository <https://github.com/sagemath/sagenb>`_.
    2025
    2126Graphics
    2227========
  • src/sage_setup/docbuild/__init__.py

    diff --git a/src/sage_setup/docbuild/__init__.py b/src/sage_setup/docbuild/__init__.py
    index 5877b5b2a7..e750d4c9ab 100644
    a b for a webpage listing all of the documents.''' % (output_dir, 
    669669
    670670        for doc in os.listdir(refdir):
    671671            directory = os.path.join(refdir, doc)
    672             if os.path.exists(os.path.join(directory, 'index.rst')):
     672            if (os.path.exists(os.path.join(directory, 'index.rst'))
     673                and os.path.join("reference", doc) not in OMIT):
    673674                n = len(os.listdir(directory))
    674675                documents.append((-n, os.path.join(self.name, doc)))
    675676
  • src/sage_setup/docbuild/build_options.py

    diff --git a/src/sage_setup/docbuild/build_options.py b/src/sage_setup/docbuild/build_options.py
    index 1168f02511..c35f71498f 100644
    a b if PAPER: 
    1616else:
    1717    PAPEROPTS = ""
    1818
     19# The legacy Sage notebook is not compatible with Python 3, and its
     20# documentation will not build with Python 3, so omit it in that case.
     21# The command line options '-t Python3' and '-t Python2' define tags
     22# Python2 and Python3 which are used by Sphinx for conditional
     23# inclusion of text: text in a block marked '.. only:: Python2' will
     24# be included when Sphinx is invoked with '-t Python2'. See
     25# http://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#including-content-based-on-tags
     26# http://www.sphinx-doc.org/en/master/man/sphinx-build.html#cmdoption-sphinx-build-t
     27SAGE_PYTHON3 = os.environ.get("SAGE_PYTHON3", "no") == "yes"
     28if SAGE_PYTHON3:
     29    TAGS = "-t Python3 "
     30    OMIT.append(os.path.join("reference", "notebook"))
     31else:
     32    TAGS = "-t Python2 "
     33
    1934#Note that this needs to have the doctrees dir
    20 ALLSPHINXOPTS = SPHINXOPTS + " " + PAPEROPTS + " "
     35ALLSPHINXOPTS = SPHINXOPTS + " " + PAPEROPTS + " " + TAGS + " "
    2136WEBSITESPHINXOPTS = ""
    2237
    2338# Number of threads to use for parallel-building the documentation.

but it doesn't work with Python 3: even though we are supposed to be ignoring the directory src/doc/en/reference/notebook, Sphinx still tries to use it in compiling indices, or something like that. Maybe there are more settings for ignoring files/directories that I could add.

Last edited 16 months ago by jhpalmieri (previous) (diff)

comment:28 Changed 16 months ago by jhpalmieri

So, you can define tags in reST which Sphinx will then pay attention to, for example

.. only:: my-tag

    stuff

Then if you run Sphinx with the option -t my-tag, stuff is included in the document. My first approach was to have tags Python2 and Python3 and run Sphinx with the corresponding -t Python[2|3]. Unfortunately, the text in stuff seems to be read even if the tag is not active, so any references to, for example, reference/notebook eventually lead to problems with Python3.

The only way I can think to get around this is to have two different index.rst files, one for use with each version of Python, and then create a symlink to the appropriate one when building the docs. One more point: any file ending in .rst in a documentation directory seems to be read by Sphinx, even if it is not explicitly referenced anywhere. So the two different index.rst files can't end in .rst. So here is a branch which implements all of this. Works for me with Python 3 (if I get around the __div__ problem discussed at #25840).

comment:29 Changed 16 months ago by git

  • Commit changed from 820100e10f17de34633447e8c4a7d235c0185dc0 to 47ca8d69724c26b0f77e0eb0167304803b83c597

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

47ca8d6trac 25382: build the reference/notebook docs with Python 2 only: skip

comment:30 Changed 16 months ago by jhpalmieri

By the way, even if this is the right approach, are symlinks the right thing to use? Are they available on Cygwin, for example? It's easy enough to copy the file instead.

comment:31 follow-up: Changed 15 months ago by gh-timokau

Can't we just not include the sagenb documentation in both cases? Besides historical baggage, I don't see why sagenb should be treated differently from other dependencies here. It would simplify this change and would also simplify future deprecation.

comment:32 in reply to: ↑ 31 Changed 15 months ago by jhpalmieri

Replying to gh-timokau:

Can't we just not include the sagenb documentation in both cases?

Hasn't been what the bulk of the discussion on this ticket has been about? It seems that not everyone agrees with this plan, so I suggested an alternative.

comment:33 Changed 15 months ago by gh-timokau

The way I read it most of this discussion is actually about weather or not we want to deprecate sagenb (#25837) and/or just disable it for python3. I don't see anybody arguing against the actual purpose of the ticket, just removing the documentation. Of course we would need to provide a user with a way to still access the documentation, just as we now do it with the jupyter notebook.

comment:34 Changed 15 months ago by git

  • Commit changed from 47ca8d69724c26b0f77e0eb0167304803b83c597 to 72c0cb2a8a0098e2f02f25c53ca6a914c402acab

Branch pushed to git repo; I updated commit sha1. New commits:

72c0cb2trac 25382: add versions of index.rst for Python2 and Python3.

comment:35 Changed 15 months ago by jhpalmieri

I think that the decision of whether to remove the docs for SageNB in Python 2 belongs on sage-devel, not here. There is also the larger question of whether SageNB should be (or is?) officially deprecated. That also belongs on sage-devel. I don't think the few participants on this ticket should be making such policy decisions on our own.

comment:36 Changed 15 months ago by fbissey

I haven't been very active in this ticket. Nevertheless I'd like to voice an opinion now. Can anyone point me to another case where package A contains the documentation for package B. It would be so much easier if sageNB was shipping its own documentation. We wouldn't even have this conversation.

comment:37 Changed 15 months ago by jhpalmieri

Of course the old Sage notebook used to be part of Sage, and (as you pointed out) it is still used as an interface to Sage. Anyway, can someone build its documentation and put is somewhere obvious on the sagenb github site? Then our reference manual could include a link to it (or skip it with Python 3, if we want conditional documentation depending on the Python version).

comment:38 Changed 15 months ago by gh-timokau

What @fbissey said was exactly the point I was also trying to make. sagenb is very much deprecated (as in no new features, minimal support, referred to as legacy in the docs) but still supported for now. #25837 discusses the future status of that.

But independently of that, I don't see why sagenb has special status. We don't include the docs of other dependencies either. We could just explain the user where he can find the docs of sagenb, which are shipped separately (as part of sagenb).

comment:39 Changed 15 months ago by jdemeyer

  • Dependencies set to #25852

We should only remove the docs if the notebook has effectively been deprecated.

comment:40 Changed 15 months ago by dimpase

Given that I am basically a glorified postdoc with funding (without anything in it earmarked for sagenb) running out in a year, I definitely do not want to spend time on sagenb. I am all for deprecating and removing it as soon as possible.

comment:41 Changed 15 months ago by dimpase

Moving the sagenb documentation to sagenb itself should be doable.

comment:42 follow-up: Changed 14 months ago by chapoton

I would prefer the simpler solution to just remove the sagenb doc inconditionally..

And if someone feels able to find another place for this doc somewhere (readthedocs ?), please do !

comment:43 Changed 14 months ago by git

  • Commit changed from 72c0cb2a8a0098e2f02f25c53ca6a914c402acab to debe3ea26348de85d79536621cc68a080d3db404

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

debe3eatrac 25382: remove the sagenb docs from the reference manual.

comment:44 Changed 14 months ago by git

  • Commit changed from debe3ea26348de85d79536621cc68a080d3db404 to 059de77749869b1a9a1e60d7ebb238316a0ac6cc

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

059de77trac 25382: remove the sagenb docs from the reference manual.

comment:45 in reply to: ↑ 42 Changed 14 months ago by jhpalmieri

Replying to chapoton:

I would prefer the simpler solution to just remove the sagenb doc inconditionally..

Like this?

comment:46 Changed 14 months ago by chapoton

yes, just like that

comment:47 Changed 14 months ago by fbissey

I won't lie. I like it. That's all that's ever needed on sage side. If someone wants to do something sagenb side, then that's another tracker.

comment:48 Changed 14 months ago by gh-timokau

I agree.

comment:49 Changed 14 months ago by jhpalmieri

I asked about the idea of removing the sagenb docs on sage-devel. Please add comments there.

comment:50 Changed 14 months ago by chapoton

  • Dependencies #25852 deleted

no need for the dependency to #25852, I think

comment:51 Changed 13 months ago by chapoton

One week has passed on sage-devel, with only positive opinions. I think it's time to set this to positive. Agreed ?

comment:52 Changed 13 months ago by fbissey

Yes. That means the paperwork needs to be done. Authors and reviewers fields need to be filled in. And the ticket set to review and so on and so forth.

comment:53 Changed 13 months ago by git

  • Commit changed from 059de77749869b1a9a1e60d7ebb238316a0ac6cc to 591e91abedeee6a7a17882f04f78e7e544f2b9d5

Branch pushed to git repo; I updated commit sha1. New commits:

591e91atrac 25382 remove remaining crosslinks

comment:54 Changed 13 months ago by chapoton

  • Authors set to John Palmieri

and we also need to check that all doctests pass..

comment:55 follow-up: Changed 13 months ago by chapoton

  • Status changed from needs_info to needs_review

This is ready for review now. Patchbot is morally green.

comment:56 Changed 13 months ago by dimpase

OK, so we should keep a copy of the ready to read sagenb docs somewhere (linked at its github project). This is now https://github.com/sagemath/sagenb/issues/460

comment:57 in reply to: ↑ 55 Changed 13 months ago by dimpase

Replying to chapoton:

This is ready for review now. Patchbot is morally green.

Are these Python3 warnings/errors from https://patchbot.sagemath.org/log/25382/Ubuntu/16.04/x86_64/4.4.0-134-generic/atlas/2018-10-17%2009:49:44?plugin=python3 harmless? Just wondering - ik habla helemaal geen Espanol...

comment:58 Changed 13 months ago by chapoton

I think that the poor patchbot (not smart enough) is troubled by the removal of the files. Or maybe by the accented letters. But this should just be noise.

comment:59 follow-up: Changed 13 months ago by dimpase

I've made a new release of sagenb, see #26499.

Should we also do downgrading sagenb to optional there?

comment:60 in reply to: ↑ 59 Changed 13 months ago by chapoton

Replying to dimpase:

I've made a new release of sagenb, see #26499.

Should we also do downgrading sagenb to optional there?

Note: the tasks of ugrading sagenb and making it optional are both fully independent of the present ticket.

comment:61 Changed 13 months ago by dimpase

On #26499 I've implemented installing sagenb (html) docs to $SAGELOCAL/share/docs/sagenb --- that's where they belong to by right.

comment:62 Changed 13 months ago by dimpase

I think that doc/en/reference/index.rst should have a link to docs of "external" packages in SAGELOCAL/share/doc (note that Sage's docs themselves are in SAGELOCAL/share/doc/sage). Then in view of #26499 one can put the link to docs in SAGELOCAL/share/doc/sagenb

comment:63 Changed 13 months ago by kcrisman

Thanks very much everyone for handling all this properly so people can still find stuff.

comment:64 Changed 13 months ago by dimpase

  • Milestone changed from sage-8.4 to sage-8.5
  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

Looks good to me. I'll modify https://github.com/sagemath/sagenb/blob/master/README.rst to point to SAGELOCAL/share/doc/sagenb as the location for docs.

We should also find a way to put it up to doc.sagemath.org, but this is also not for this ticket.

comment:65 Changed 13 months ago by dcoudert

FYI: graph_editor.py imports sagenb.notebook.interact, but has no pointer to the documentation of sagenb.

We will certainly have to remove this tool when sagenb will be removed.

comment:66 Changed 13 months ago by gh-timokau

@dcoudert also see #25837.

comment:67 Changed 13 months ago by vbraun

  • Branch changed from public/25382 to 591e91abedeee6a7a17882f04f78e7e544f2b9d5
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:68 Changed 10 months ago by kcrisman

  • Commit 591e91abedeee6a7a17882f04f78e7e544f2b9d5 deleted

Unfortunately now it seems that the interact documentation is not in Sage's usual online doc. That will break a number of links out there, plus interact is in many different interfaces to Sage, not just sagenb.

comment:69 Changed 10 months ago by fbissey

Well just like sagenb documentation should not be shipped with sage, sage documentation should not be in sagenb. That means some documentation has be migrated/rewritten in sage proper.

comment:70 Changed 10 months ago by dimpase

sagenb docs are installed in SAGE_LOCAL/share/doc/, among with many others.

This is however not at all reflected in the reference manual. I'm not sure how to achieve this with sphinx. By relative paths, like ../../ ?

comment:71 Changed 10 months ago by gh-timokau

Not sure how its done right now, but intersphinx may be worth looking at: http://www.sphinx-doc.org/en/master/usage/extensions/intersphinx.html

comment:72 Changed 10 months ago by dimpase

I agree that intersphinx looks like the right tool for the job.

comment:73 Changed 10 months ago by kcrisman

In any case, are we agreed that the interact documentation should be visible now?

Note: See TracTickets for help on using tickets.