Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

configure --disable-notebook: Also disable jupyter_sphinx #38224

Merged
merged 12 commits into from
Sep 3, 2024

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Jun 15, 2024

jupyter_sphinx is only needed for the live documentation and for some illustrations in src/doc/en/tutorial/latex.rst.

We conditionalize the use of this extension using a Feature.

configure --disable-notebook now also disables jupyter_sphinx, to avoid pulling in lots of Jupyter packages via its dependencies (including jsonschema, which has gotten itself a Rust-based dependency, see #38219).

This implements the policy for use of platform-dependent wheels proposed in #38219 ("if a platform-dependent wheel package is a standard package, there must be a configure option that disables it").

We also repair the mechanism for conditional documentation based on feature tags.
In particular, the link for Boolean polynomials is restored. https://doc-pr-38224--sagemath.netlify.app/html/en/reference/polynomial_rings/#boolean-polynomials

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 15, 2024

Need to conditionalize .. JUPYTER-EXECUTE in src/doc/en/tutorial/latex.rst using .. ONLY - https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#including-content-based-on-tags

@mkoeppe mkoeppe force-pushed the jupyter_sphinx_optional branch 3 times, most recently from a304a76 to a33d5de Compare June 17, 2024 17:18
Copy link

github-actions bot commented Jun 17, 2024

Documentation preview for this PR (built with commit 33a114c; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@orlitzky
Copy link
Contributor

This beta's no better than the last one for me:

[sagelib-10.4.rc0] [spkg-install] Updating Cython code....
[sagelib-10.4.rc0] [spkg-install] ************************************************************************
[sagelib-10.4.rc0] [spkg-install] Traceback (most recent call last):
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/pyproject_hooks/_in_process/_in_process.py", line 373, in <module>
[sagelib-10.4.rc0] [spkg-install]     main()
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/pyproject_hooks/_in_process/_in_process.py", line 357, in main
[sagelib-10.4.rc0] [spkg-install]     json_out["return_val"] = hook(**hook_input["kwargs"])
[sagelib-10.4.rc0] [spkg-install]                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/pyproject_hooks/_in_process/_in_process.py", line 271, in build_wheel
[sagelib-10.4.rc0] [spkg-install]     return _build_backend().build_wheel(
[sagelib-10.4.rc0] [spkg-install]            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/build_meta.py", line 410, in build_wheel
[sagelib-10.4.rc0] [spkg-install]     return self._build_with_temp_dir(
[sagelib-10.4.rc0] [spkg-install]            ^^^^^^^^^^^^^^^^^^^^^^^^^^
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/build_meta.py", line 395, in _build_with_temp_dir
[sagelib-10.4.rc0] [spkg-install]     self.run_setup()
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/build_meta.py", line 311, in run_setup
[sagelib-10.4.rc0] [spkg-install]     exec(code, locals())
[sagelib-10.4.rc0] [spkg-install]   File "<string>", line 95, in <module>
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/__init__.py", line 103, in setup
[sagelib-10.4.rc0] [spkg-install]     return distutils.core.setup(**attrs)
[sagelib-10.4.rc0] [spkg-install]            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/_distutils/core.py", line 184, in setup
[sagelib-10.4.rc0] [spkg-install]     return run_commands(dist)
[sagelib-10.4.rc0] [spkg-install]            ^^^^^^^^^^^^^^^^^^
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/_distutils/core.py", line 200, in run_commands
[sagelib-10.4.rc0] [spkg-install]     dist.run_commands()
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/_distutils/dist.py", line 969, in run_commands
[sagelib-10.4.rc0] [spkg-install]     self.run_command(cmd)
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/dist.py", line 968, in run_command
[sagelib-10.4.rc0] [spkg-install]     super().run_command(command)
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/_distutils/dist.py", line 988, in run_command
[sagelib-10.4.rc0] [spkg-install]     cmd_obj.run()
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/wheel/bdist_wheel.py", line 368, in run
[sagelib-10.4.rc0] [spkg-install]     self.run_command("build")
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/_distutils/cmd.py", line 316, in run_command
[sagelib-10.4.rc0] [spkg-install]     self.distribution.run_command(command)
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/dist.py", line 968, in run_command
[sagelib-10.4.rc0] [spkg-install]     super().run_command(command)
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/_distutils/dist.py", line 988, in run_command
[sagelib-10.4.rc0] [spkg-install]     cmd_obj.run()
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/_distutils/command/build.py", line 132, in run
[sagelib-10.4.rc0] [spkg-install]     self.run_command(cmd_name)
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/_distutils/cmd.py", line 316, in run_command
[sagelib-10.4.rc0] [spkg-install]     self.distribution.run_command(command)
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/dist.py", line 968, in run_command
[sagelib-10.4.rc0] [spkg-install]     super().run_command(command)
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/_distutils/dist.py", line 988, in run_command
[sagelib-10.4.rc0] [spkg-install]     cmd_obj.run()
[sagelib-10.4.rc0] [spkg-install]   File "/home/mjo/src/sage.git/pkgs/sagemath-standard/sage_setup/command/sage_build_ext.py", line 25, in run
[sagelib-10.4.rc0] [spkg-install]     self.run_command('build_cython')
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/_distutils/cmd.py", line 316, in run_command
[sagelib-10.4.rc0] [spkg-install]     self.distribution.run_command(command)
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/dist.py", line 968, in run_command
[sagelib-10.4.rc0] [spkg-install]     super().run_command(command)
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/setuptools/_distutils/dist.py", line 988, in run_command
[sagelib-10.4.rc0] [spkg-install]     cmd_obj.run()
[sagelib-10.4.rc0] [spkg-install]   File "/home/mjo/src/sage.git/pkgs/sagemath-standard/sage_setup/command/sage_build_cython.py", line 220, in run
[sagelib-10.4.rc0] [spkg-install]     extensions = cythonize(
[sagelib-10.4.rc0] [spkg-install]                  ^^^^^^^^^^
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/site-packages/Cython/Build/Dependencies.py", line 1150, in cythonize
[sagelib-10.4.rc0] [spkg-install]     result.get(99999)  # seconds
[sagelib-10.4.rc0] [spkg-install]     ^^^^^^^^^^^^^^^^^
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/multiprocessing/pool.py", line 774, in get
[sagelib-10.4.rc0] [spkg-install]     raise self._value
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/multiprocessing/pool.py", line 540, in _handle_tasks
[sagelib-10.4.rc0] [spkg-install]     put(task)
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/multiprocessing/connection.py", line 206, in send
[sagelib-10.4.rc0] [spkg-install]     self._send_bytes(_ForkingPickler.dumps(obj))
[sagelib-10.4.rc0] [spkg-install]                      ^^^^^^^^^^^^^^^^^^^^^^^^^^
[sagelib-10.4.rc0] [spkg-install]   File "/usr/lib/python3.12/multiprocessing/reduction.py", line 51, in dumps
[sagelib-10.4.rc0] [spkg-install]     cls(buf, protocol).dump(obj)
[sagelib-10.4.rc0] [spkg-install] AttributeError: Can't pickle local object '_prepare_extension_detection.<locals>.<lambda>'
[sagelib-10.4.rc0] [spkg-install] ************************************************************************
[sagelib-10.4.rc0] [spkg-install] Error building the Sage library
[sagelib-10.4.rc0] [spkg-install] ************************************************************************
[sagelib-10.4.rc0] [spkg-install] Please email sage-devel (http://groups.google.com/group/sage-devel)
[sagelib-10.4.rc0] [spkg-install] explaining the problem and including the relevant part of the log file
[sagelib-10.4.rc0] [spkg-install]   
[sagelib-10.4.rc0] [spkg-install] Describe your computer, operating system, etc.
[sagelib-10.4.rc0] [spkg-install] ************************************************************************
[sagelib-10.4.rc0] [spkg-install] 
[sagelib-10.4.rc0] [spkg-install] ERROR Backend subprocess exited when trying to invoke build_wheel
[sagelib-10.4.rc0] [spkg-install] ********************************************************************************
[sagelib-10.4.rc0] [spkg-install] Error building a wheel for sagelib-10.4.rc0
[sagelib-10.4.rc0] [spkg-install] ********************************************************************************
[sagelib-10.4.rc0] ************************************************************************
[sagelib-10.4.rc0] Error installing package sagelib-10.4.rc0
[sagelib-10.4.rc0] ************************************************************************

I'm going back outside.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 23, 2024

I think @kiwifb can tell you about this gentoo-specific problem

@kiwifb
Copy link
Member

kiwifb commented Jun 23, 2024

@orlitzky you are officially bitten by cschwan/sage-on-gentoo#783 the good news is that upstream accepted to fix it and an appropriate version of scikit-build-core is in the gentoo tree.

@orlitzky
Copy link
Contributor

@orlitzky you are officially bitten by cschwan/sage-on-gentoo#783 the good news is that upstream accepted to fix it and an appropriate version of scikit-build-core is in the gentoo tree.

Yep, that was it. Upgrading fixed it. Thanks!

@orlitzky
Copy link
Contributor

Further, at least:

[sagelib-10.4.rc0] Finished installing sagelib-10.4.rc0
[sagelib-10.4.rc0] real 1m23.544s user 1m15.795s sys 0m9.496s
make[2]: Leaving directory '/home/mjo/src/sage.git/build/make'
Sage build/upgrade complete!
real 1m25.445s user 1m19.133s sys 0m11.506s
make[1]: Leaving directory '/home/mjo/src/sage.git'
mjo@gantu ~/src/sage.git $ ./sage
************************************************************************
It seems that you are attempting to run Sage from an unpacked source
tarball, but you have not compiled it yet (or maybe the build has not
finished). You should run `make` in the SAGE_ROOT directory first.
If you did not intend to build Sage from source, you should download
a binary tarball instead. Read README.txt for more information.
************************************************************************

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 9, 2024

This implements the policy for use of platform-dependent wheels proposed in #38219 ("if a platform-dependent wheel package is a standard package, there must be a configure option that disables it").

Is there a discussion on this policy? What is the rationale of the policy? It (disabling a standard package) sounds like a oxymoron to me...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 9, 2024

To me, a standard package is simply one that is configured to be installed by default.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 9, 2024

"disabling standard package" sounds strange but is okay. My question is rather on "must be". Why "must be a configure option" for "platform-dependent wheel"?

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 9, 2024

I get

sage: from sage.features.sphinx import JupyterSphinx
sage: JupyterSphinx().is_present()
FeatureTestResult('jupyter_sphinx', True)

but the part covered by .. ONLY:: html and feature_jupyter_sphinx is missing in the built document. Is this normal?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 9, 2024

Why "must be a configure option" for "platform-dependent wheel"?

Currently, standard Sage can be built from source, so (at least in theory) on a wide range of architectures. If we allow standard packages to be provided by platform-dependent wheels, that restricts such packages to the fixed range of architectures for which wheels have been provided. This could be highly controversial, which is why I am proposing the conservative policy in #38219 that such packages at least must have a --disable-... option.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 9, 2024

Why "must be a configure option" for "platform-dependent wheel"?

Currently, standard Sage can be built from source, so (at least in theory) on a wide range of architectures. If we allow standard packages to be provided by platform-dependent wheels, that restricts such packages to the fixed range of architectures for which wheels have been provided.

I understand up to this point.

This could be highly controversial, which is why I am proposing the conservative policy in #38219 that such packages at least must have a --disable-... option.

Let's assume a platform on which the standard package has no wheel yet. What should happen to the package when sage is built on that platform? Failure? So --disable-... option?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 10, 2024

This could be highly controversial, which is why I am proposing the conservative policy in #38219 that such packages at least must have a --disable-... option.

Let's assume a platform on which the standard package has no wheel yet.

Not just "not yet": Making wheels for the packages is not under the control of our project.

What should happen to the package when sage is built on that platform? Failure?

Yes, it's a hard error.

So --disable-... option?

Exactly.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 14, 2024

This PR gives me a philosophical confusion about the meaning of standard packages in sage.

A standard package is installed by default. Hence code in sage assumes its existence (or rather the functions provided by the package). The code in this PR allows the possibility that the feature provided by a standard package may not exist.

Don't we have "optional package" just for the case? I think there is no essential difference between (S) standard package that may be disabled and (O) optional package that may be enabled.

On the other hand, I am aware that similar blurring was already introduced into the sage library through the modularization...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 14, 2024

A standard package is installed by default.

Exactly, and that's the complete answer.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 14, 2024

Don't we have "optional package" just for the case?

No, an optional package is not installed by default.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 14, 2024

A standard package is installed by default.

Exactly, and that's the complete answer.

Do we have a precedence that a standard package can be disabled and the possibility is reflected in sage code?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 14, 2024

Yes, there are a bunch of --disable-... options, see https://github.com/sagemath/sage/blob/develop/configure.ac#L442

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 14, 2024

and the possibility is reflected in sage code?

Yes, you can find a bunch of # needs cvxopt

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 14, 2024

Package-wise, I see only three relevant cases: --disable-cvxopt, --disable-notebook, --disable-r.

R is an optional package. I don't understand why --disable-r exists. Perhaps just an overlook.

So cvxopt and notebook are relevant examples of standard packages that can be disabled. I think these are rather exceptions, made for practical purposes.

If an arbitrary standard package can be disabled in principle, then code in sage should spend time checking its availability before doing anything useful. For example, Singular is a standard package, and the code in sage relying on Singular can assume its existence. I cannot agree with your definition of standard packages.

Since notebook is already an exception, perhaps jupyter-sphinx may be treated also as an exception -- standard package that can be disabled and hence needs to be checked for its availability. We may call this "semi-standard"...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 14, 2024

R is an optional package. I don't understand why --disable-r exists. Perhaps just an overlook.

--disable-r also disables rpy2, which is a standard package. (Actually a "semi-standard" package -- rpy2 is automatically disabled when there is no system r.)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 14, 2024

If an arbitrary standard package can be disabled in principle, then code in sage should spend time checking its availability before doing anything useful. For example, Singular is a standard package, and the code in sage relying on Singular can assume its existence.

Yes, and the various # needs tags introduced in the modularization do exactly this.

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working well. LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 15, 2024

Thank you!

vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 27, 2024
…er_sphinx`

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

`jupyter_sphinx` is only needed for the live documentation and for some
illustrations in `src/doc/en/tutorial/latex.rst`.

We conditionalize the use of this extension using a Feature.

`configure --disable-notebook` now also disables `jupyter_sphinx`, to
avoid pulling in lots of Jupyter packages via its dependencies
(including `jsonschema`, which has gotten itself a Rust-based
dependency, see sagemath#38219).

This implements the policy for use of platform-dependent wheels proposed
in sagemath#38219 ("if a platform-dependent ``wheel`` package is a standard
package, there must be a ``configure`` option that disables it").

We also repair the mechanism for conditional documentation based on
feature tags.
In particular, the link for Boolean polynomials is restored.
https://doc-pr-38224--
sagemath.netlify.app/html/en/reference/polynomial_rings/#boolean-
polynomials

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

- Depends on sagemath#38468 (merged here)
    
URL: sagemath#38224
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
@vbraun vbraun merged commit fc80abe into sagemath:develop Sep 3, 2024
17 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants