Opened 3 years ago

Last modified 3 years ago

#21732 needs_work enhancement

Move runtime documentation python modules into src/sage

Reported by: infinity0 Owned by:
Priority: major Milestone: sage-7.6
Component: documentation Keywords: days85
Cc: hivert Merged in:
Authors: Erik Bray Reviewers:
Report Upstream: N/A Work issues:
Branch: u/embray/ticket-21732 (Commits) Commit: 77b5428908e8aff455dabbfad9eb63139ce89ef9
Dependencies: #22611 #22655 Stopgaps:

Description (last modified by embray)

Currently, (1) at runtime sage requires some files from under SAGE_DOC_SRC. For example:

$ git grep SAGE_DOC_SRC -- src/sage
[..]
src/sage/misc/sagedoc.py:    sys.path = [os.path.join(SAGE_DOC_SRC, 'common')] + oldpath
[..]
src/sage/misc/sphinxify.py:    confdir = os.path.join(SAGE_DOC_SRC, 'en', 'introspect')

However, this is awkward for binary distros such as Debian, who don't install source code onto end user machines by default. Indeed, even Sage's own Makefiles don't install these files into SAGE_LOCAL, even though they are a runtime necessity.

In addition to this, (2) there are many cases where Sage tracks SAGE_DOC_SRC and hacks it into sys.path via os.environ, which is not very clean.

This ticket proposes the following change (or something similar)

  • src/doc/common/* -> src/sage/docs/common/*
  • src/doc/en/introspect/* -> src/sage/docs/introspect/*

This would solve the above two issues - instead of (2) one can just do from sage.doc.common import conf or from sage.doc.introspect import conf, and (1) is taken care of automatically because everything under src/sage is installed as standard python modules.

One could also remove the OMIT = ["introspect"] exception in src/sage_setup/docbuild/build_options.py.

Does this sound sensible? If so I can do this for our Debian packaging already, test it, and send in an initial patch.

Change History (51)

comment:1 Changed 3 years ago by jdemeyer

  • Summary changed from Move runtime documentation python modules into sagelib to Move documentation sources into src/sage

Putting the documentation as top-level directory of the project is the standard thing to do with Sphinx. We aren't quite using Sphinx in the standard way as there is a lot of Sage customization, but still... I see no reason to be even more different.

Just saying "it would make our lives easier to do X" is not a good explanation. You really need to justify better why you want to do X.

comment:2 follow-up: Changed 3 years ago by infinity0

Why is reducing development cost not a good explanation? None of the files I mentioned are actual direct sphinx config files, they are all meant to be included or used by something else. So they are already not being used in a "standard Sphinx way".

comment:3 Changed 3 years ago by infinity0

  • Summary changed from Move documentation sources into src/sage to Move runtime documentation python modules into src/sage

Changing the title back because the ticket is not about moving all documentation sources.

comment:4 in reply to: ↑ 2 Changed 3 years ago by jdemeyer

Replying to infinity0:

Why is reducing development cost not a good explanation?

If you use an argument of the form "I want X because it helps Y" and we both agree that Y is a good thing, you still need to explain why X helps with Y. And I am totally missing that.

And this title is totally confusing me, I hardly see how it relates to moving src/doc/common. It might relate to moving the introspect doc, but then the title covers only a part of the ticket.

comment:5 follow-up: Changed 3 years ago by infinity0

It reduces development cost because you no longer have to track SAGE_DOC_SRC in many places and hack it into sys.path from os.environ. You can just do from sage.doc.common import conf or from sage.doc.introspect import conf. This helps with tickets like #21495 which is what we ideally need in Debian (but currently need to hack the equivalent functionality in via patches).

In additional to reducing development cost, this change (or something similar) is *necessary* for binary distributions because we normally don't install things like SAGE_SRC or SAGE_DOC_SRC onto end user machines, yet the files I mentioned are needed at runtime - and only these files, not the other conf.py files or doc sources.

Last edited 3 years ago by infinity0 (previous) (diff)

comment:6 Changed 3 years ago by infinity0

$ git grep SAGE_DOC_SRC -- src/sage
[..]
src/sage/misc/sagedoc.py:    sys.path = [os.path.join(SAGE_DOC_SRC, 'common')] + oldpath
[..]
src/sage/misc/sphinxify.py:    confdir = os.path.join(SAGE_DOC_SRC, 'en', 'introspect')

are the relevant lines.

comment:7 in reply to: ↑ 5 Changed 3 years ago by jdemeyer

Replying to infinity0:

It reduces development cost because you no longer have to track SAGE_DOC_SRC in many places and hack it into sys.path from os.environ. You can just do from sage.doc.common import conf or from sage.doc.introspect import conf. This helps with tickets like #21495 which is what we ideally need in Debian (but currently need to hack the equivalent functionality in via patches).

This explanation should be in the ticket description. There is no way I could figure out the above paragraph from just reading the description.

comment:8 Changed 3 years ago by infinity0

  • Description modified (diff)

Alright, sorry, I'll take some more time writing future tickets. I've edited the description now, hopefully it's OK.

comment:9 Changed 3 years ago by jdemeyer

At least I understand the problem now. Still, I don't know the best way to fix it.

comment:10 Changed 3 years ago by jdemeyer

  • Cc hivert added

comment:11 Changed 3 years ago by mkoeppe

I agree that sage at runtime should not refer to anything in SAGE_DOC_SRC. Whatever is needed from there should be installed in an appropriate place in SAGE_LOCAL.

comment:12 Changed 3 years ago by embray

I noticed this recently too--glad to see there's already an issue for it. I agree, using a location in SAGE_LOCAL would be best.

comment:13 Changed 3 years ago by embray

I'm starting to agree it would be good if this common doc stuff just went in a sage.misc.docs sub-package or something. As it is common/conf.py imports from sage so it already has a dependency on it anyways.

This would be a good move toward the previously-discussed ideal of making it easier for third-party packages to use Sage's documentation utilities.

Another options which I've brought up before is to move all docs-related utilities to a separate sage_documention package (to which sage_setup.docbuild would also be moved). This would necessarily be a runtime dependency of sage for docstring sphinxifying to work.

comment:14 Changed 3 years ago by embray

  • Dependencies set to #22061

I'm working on a possible solution to this, but I believe that any reasonable solution should depend on #22061 at a minimum.

comment:15 Changed 3 years ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/ticket-21732
  • Commit set to 6194785e2d9965614f5698a7538026b1f854e454

Here's my attempt at solving this. It included a few other changes that I felt were needed (in addition to #22061), but that are not directly relevant either, so might be worth moving to another ticket.

I don't feel strongly about the choice of sage.misc.docs, but I am convinced that these files should be somewhere (for now) in the sage package.


New commits:

9175036Don't hard-code (in the form of a symlink) the path to thebe.js.
ba840c6A couple enhancements to find_extra_files:
9e1bdc3Adds a sage_build_py command in setup.py
334da54Fix some tests that broken in sage_setup
2983351Fully move src/doc/common and src/doc/en/introspect to subpackages of a new sage.misc.docs subpackage.
31a3989Fix no longer accurate reference to SAGE_DOC_SRC in a docstring
6194785As mentioned in #21732, 'introspect' is no longer needed in OMIT, as 'introspect' is no longer in the doc source tree.

comment:16 Changed 3 years ago by embray

  • Status changed from new to needs_review

comment:17 follow-up: Changed 3 years ago by infinity0

In here https://git.sagemath.org/sage.git/diff/src/sage/misc/sphinxify.py?id=2983351b1116ed29f08d5fc95c6c11dbcddeec64 and possible some of the other changes, don't we want SAGE_LOCAL instead of SAGE_SRC? (Assuming we don't want to always install SAGE_SRC at runtime.)

comment:18 Changed 3 years ago by infinity0

er, I mean SAGE_LIB instead of SAGE_LOCAL (or SAGE_SRC)

comment:19 in reply to: ↑ 17 Changed 3 years ago by embray

Replying to infinity0:

In here https://git.sagemath.org/sage.git/diff/src/sage/misc/sphinxify.py?id=2983351b1116ed29f08d5fc95c6c11dbcddeec64 and possible some of the other changes, don't we want SAGE_LOCAL instead of SAGE_SRC? (Assuming we don't want to always install SAGE_SRC at runtime.)

Yes, this is true. Actually, I have another branch where I'm trying to improve runtime dependency on SAGE_SRC (or, more specifically, SAGE_SRC is set to the same as SAGE_LIB when not running out of the source tree).

I guess this change is a holdover from that. But at the same time I did this work as a prerequisite for fixing the other runtime dependencies on SAGE_DOC_SRC and SAGE_SRC, so I guess there's an interdependency between the two.

Maybe, for the sake of making this ticket make sense on its own, I'll bring in some of the changes from my other branch too.

comment:20 follow-up: Changed 3 years ago by kcrisman

See also sagenb PR 416. Perhaps an update ticket for that is needed too.

comment:21 in reply to: ↑ 20 ; follow-up: Changed 3 years ago by embray

Replying to kcrisman:

See also sagenb PR 416. Perhaps an update ticket for that is needed too.

You mean, to update the sagenb version in the distribution?

comment:22 in reply to: ↑ 21 Changed 3 years ago by kcrisman

See also sagenb PR 416. Perhaps an update ticket for that is needed too.

You mean, to update the sagenb version in the distribution?

Correct.

comment:23 Changed 3 years ago by embray

Another possibility would be to keep a symlink to sage/misc/docs/introspect in its old location ($SAGE_DOC_SRC/en/introspect). That would at least preserve backwards-compat, I think.

comment:24 Changed 3 years ago by jdemeyer

  • Keywords days85 added
  • Status changed from needs_review to needs_work

Does not merge.

comment:25 Changed 3 years ago by hivert

  • Branch changed from u/embray/ticket-21732 to u/hivert/ticket-21732

comment:26 Changed 3 years ago by hivert

  • Commit changed from 6194785e2d9965614f5698a7538026b1f854e454 to 8c3b6a10dc67f33d58a897bc928b9db145e93d9f

I Handled the (trivial) merge.

Florent


New commits:

8c3b6a1Merged with 7.6.beta6

comment:27 Changed 3 years ago by hivert

  • Status changed from needs_work to needs_review

comment:28 Changed 3 years ago by chapoton

  • Milestone changed from sage-7.5 to sage-7.6
  • Status changed from needs_review to needs_work

needs rebase on 7.6.rc0

comment:29 Changed 3 years ago by git

  • Commit changed from 8c3b6a10dc67f33d58a897bc928b9db145e93d9f to 1e372a8e0cd5cab6523f6cafba7698bfa0761a07

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

1e372a8Merge branch 'develop' into t/21732/ticket-21732

comment:30 Changed 3 years ago by hivert

  • Status changed from needs_work to needs_review

comment:31 Changed 3 years ago by hivert

I've reviewed this patch and it look quite good to me (though I'm not very competent about the necessity of this changes for better packaging in distros). I still have a few remark

  • I agree with the comment in the head of commit 1e372a8 that the moving of module_list.py into sage_setup should go into another ticket.
  • I would have put thing under something like sage/doc-conf or sage/doc-script. I'm not too happy putting this in misc...
  • In sage_setup/docbuild/__init__.py, it says:
    The sphinx subprocesses are configured in conf.py
    
    I'd rather put here a comment explaining where to find this conf.py and why it was moved there.
  • Finally, we should take the opportunity to update python.inv.

I'm not sure I'm completely competent to put a positive review here. And I'd rather have someone else looking quickly.

comment:32 Changed 3 years ago by infinity0

Erik suggested in 19 that he might make some further tweaks, are these still planned Erik?

comment:33 Changed 3 years ago by embray

This is a case of me not even remembering what I was doing 3 months ago, so I'll have to take a bit to remind myself.

I do remember that I was working on the larger task of eliminating any runtime dependencies on SAGE_SRC (or, put more accurately, allowing SAGE_SRC to point to site-packages/sage in some cases).

comment:34 Changed 3 years ago by embray

I think I see what I meant now in 19. It will make more sense if I post a ticket for my other branch with the follow-up work. I think the question was whether or not to include the one change you mentioned in 17 that doesn't otherwise seem obvious on its own.

comment:35 follow-up: Changed 3 years ago by jdemeyer

I don't see why this needs to make so much changes to the Sage build system which look unrelated to the documentation. I think those changes should certainly be on a new ticket.

comment:36 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

And then a bikeshed issue: I would suggest src/sage/docs instead of src/sage/misc/docs.

comment:37 Changed 3 years ago by embray

hivert wasn't crazy about putting it in misc.docs either. sage.docs works for me.

comment:38 in reply to: ↑ 35 ; follow-up: Changed 3 years ago by embray

Replying to jdemeyer:

I don't see why this needs to make so much changes to the Sage build system which look unrelated to the documentation. I think those changes should certainly be on a new ticket.

The other changes are necessary because of the poor way we currently handle non-Python files in packages, and I made some improvements there. You're right though that it's not directly related; rather it was a prerequisite. I could create a separate ticket if you prefer and move those changes. Then this ticket would become dependent on the new ticket.

comment:39 in reply to: ↑ 38 Changed 3 years ago by jdemeyer

Replying to embray:

Replying to jdemeyer:

I don't see why this needs to make so much changes to the Sage build system which look unrelated to the documentation. I think those changes should certainly be on a new ticket.

The other changes are necessary because of the poor way we currently handle non-Python files in packages, and I made some improvements there.

I'm not really convinced that you need to make such big changes (like moving module_list.py). But if you do make those changes, I think they should be on a different ticket.

comment:40 Changed 3 years ago by fbissey

I am not sure why I wasn't on this ticket before. Off course I suffer from this and my changes are much more minimalistic overall. I have to check more of the stuff to see if it is really useful to sage-on-distro in general. Note that moving module_list.py makes me scratch my head? Why? Like all the files in the src folder it is not actually installed. More consistency? May be. Breaking every sage-on-distro patch against module_list.py? Yes. Not difficult to deal with, just annoying.

comment:41 Changed 3 years ago by fbissey

Overall I like the changes that pertains to the documentation. Like Jeroen I would prefer the other bits touching setup.py, find.py and so on. I would prefer it being another ticket with a separate rational. If it is necessary I need more info. I can be very stupid that way.

Speaking of hacks, a related issue that beeped on my radar reading the ticket. We replaced SAGE_DOC_SRC by SAGE_SRC in a few place, including sphinxify.py. Me and probably most of the other distros point SAGE_SRC to SAGE_LIB at runtime. However it is not desirable to point to your sources at runtime. We may need a better mechanism in a follow up ticket.

comment:42 follow-up: Changed 3 years ago by fbissey

I knew I had forgotten something. There is a lot of stuff in sage/misc I think stuff is coherent enough that it could go in sage/docs rather than sage/misc/docs but I am not going to insist on it if it takes to much time to move things.

comment:43 in reply to: ↑ 42 Changed 3 years ago by jdemeyer

Replying to fbissey:

I knew I had forgotten something. There is a lot of stuff in sage/misc I think stuff is coherent enough that it could go in sage/docs rather than sage/misc/docs

I agree but let's do that in a new ticket. There is already too much happening in this ticket. I suggest to move the files from src/doc to src/sage/docs but to keep the existing doc-related files in src/sage/misc for now.

comment:44 Changed 3 years ago by embray

Okay, well, there were definitely good reasons for the other changes, but it was long enough ago now that I don't remember what they all were. Let me see if I can break it off to a separate ticket where I explain them better.

Moving module_list.py might not have been necessary, but I don't recall (eventually I think it should go away entirely but that's another subject).

comment:45 follow-up: Changed 3 years ago by embray

jdemeyer: You had mentioned that there was some other work you were doing that this ticket should depend on? Is there a branch for that posted yet, that I could base on? Or is mainly just a matter of naming the package in sage sage.doc (or is it sage.docs)?

comment:46 in reply to: ↑ 45 Changed 3 years ago by jdemeyer

  • Dependencies changed from #22061 to #22611

Replying to embray:

jdemeyer: You had mentioned that there was some other work you were doing that this ticket should depend on? Is there a branch for that posted yet, that I could base on? Or is mainly just a matter of naming the package in sage sage.doc (or is it sage.docs)?

There is at least #22611, which creates the sage.docs directory.

You should also be aware of #22252 which affects docbuilding. Depending on how well git can deal with renames, this will probably not actually conflict.

comment:47 Changed 3 years ago by embray

Right, I think you had mentioned both of those. Thanks!

comment:48 Changed 3 years ago by embray

  • Branch changed from u/hivert/ticket-21732 to u/embray/ticket-21732
  • Commit changed from 1e372a8e0cd5cab6523f6cafba7698bfa0761a07 to 77b5428908e8aff455dabbfad9eb63139ce89ef9
  • Dependencies changed from #22611 to #22611 #22655
  • Description modified (diff)

New branch for this ticket based on #22655, which separates out the build changes to be considered separately. This now just makes the code moves suggested in the description of the ticket.

To be clear, nothing substantive changed in these files except to update some imports, and remove bits of code that were previously needed to manipulate sys.path. Now functions like sphinxify can work without SAGE_DOC_SRC needing to be installed somewhere on the system.


New commits:

e404a68A couple enhancements to find_extra_files:
5d30652Adds a sage_build_py command in setup.py
77b5428Move common documentation config and template files from `src/doc` to a new

comment:49 Changed 3 years ago by embray

Also, as already noted, this will likely conflict with #22611, but I'm fine waiting for that to be merged first.

comment:50 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

comment:51 Changed 3 years ago by chapoton

  • Status changed from needs_review to needs_work

does not apply

Note: See TracTickets for help on using tickets.