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

py3.9 support cleanup in sage_autodoc and sphinx bump #39577

Merged
merged 2 commits into from
Feb 28, 2025

Conversation

kiwifb
Copy link
Member

@kiwifb kiwifb commented Feb 24, 2025

This is a follow up to #39251
This PR removes work around to support older python during the last two synchronization of sage_autodoc.py with upstream. python 3.9/3.10 support removal also enable us to move to a newer version of sphinx.

📝 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

… synchronization of sage_autodoc.py with upstream
@kiwifb kiwifb requested a review from tobiasdiez February 24, 2025 00:37
Copy link

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

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.

LGTM. Thanks.

Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@vbraun vbraun merged commit 3918083 into sagemath:develop Feb 28, 2025
21 of 49 checks passed
@dimpase
Copy link
Member

dimpase commented Mar 10, 2025

@kiwifb with sphinx 8.2.3 I had to apply

--- a/src/sage_docbuild/ext/sage_autodoc.py
+++ b/src/sage_docbuild/ext/sage_autodoc.py
@@ -2976,7 +2976,7 @@ class PropertyDocumenter(DocstringStripSignatureMixin,  # type: ignore[misc]
 
 def autodoc_attrgetter(app: Sphinx, obj: Any, name: str, *defargs: Any) -> Any:
     """Alternative getattr() for types"""
-    for typ, func in app.registry.autodoc_attrgetters.items():
+    for typ, func in app.registry.autodoc_attrgettrs.items():
         if isinstance(obj, typ):
             return func(obj, name, *defargs)
 

Was it renamed in 8.2, or it's a typo in the branch?

@kiwifb
Copy link
Member Author

kiwifb commented Mar 10, 2025

Seems familiar, I am fairly sure that's a renaming in 8.2. Let me check.

@kiwifb
Copy link
Member Author

kiwifb commented Mar 10, 2025

OK I see it on the branch. I may have picked the wrong branch of the work around. Looking sphinx.

@kiwifb
Copy link
Member Author

kiwifb commented Mar 10, 2025

Done my archeology. Yes I picked the wrong branch in the try except. It is part of a commit about typos upstream sphinx-doc/sphinx@9323de2#diff-e43bdd6f8f37a12d2536e09e57c5e8999cb8de18b9c7ba49126f90576c4328ac but there have been further changes in 8.2, good job that it still builds without more changes sphinx-doc/sphinx@f3e0de1#diff-e43bdd6f8f37a12d2536e09e57c5e8999cb8de18b9c7ba49126f90576c4328ac

@dimpase
Copy link
Member

dimpase commented Mar 11, 2025

it's rather confusing. Which way are they moving to? They still have both spellings...

@kiwifb
Copy link
Member Author

kiwifb commented Mar 11, 2025

Given the direction sphinx-doc/sphinx#11936 I would say any instances of attrgettrs is a typo to be fixed.

@kiwifb
Copy link
Member Author

kiwifb commented Mar 11, 2025

It looks like there is an alias in place, at least in head, to match the two spellings https://github.com/search?q=repo%3Asphinx-doc%2Fsphinx%20attrgettrs&type=code and it was added as part of the PR above in sphinx. It seems more and more curious as to why you had to patch.

@dimpase
Copy link
Member

dimpase commented Mar 11, 2025

can we install such an alias in our docs?

@kiwifb
Copy link
Member Author

kiwifb commented Mar 11, 2025

I thought why not but now, I am not so sure. I am half surprised we do not benefit from sphinx's one. I need to get sphinx-8.2+ installed and see what happens for myself.

@dimpase
Copy link
Member

dimpase commented Mar 11, 2025

I see this def in registry.py in the installation of sphinx 8.2.3 (this is an up to date Gentoo install, by the way)
It has the following, and I don't think it's an alias, it's the only way to access the dict autodoc_attrgetters.

class SphinxComponentRegistry:
    def __init__(self) -> None:
        #: special attrgetter for autodoc; class object -> attrgetter
        self.autodoc_attrgetters: dict[type, Callable[[Any, str, Any], Any]] = {}
[...]
    @property
    def autodoc_attrgettrs(self) -> dict[type, Callable[[Any, str, Any], Any]]:
        return self.autodoc_attrgetters

I never recall how exactly Python classes work, but you see that self.autodoc_attrgetters sits inside __init__, but autodoc_attrgettrs is a proper member function of the class SphinxComponentRegistry.

If I try to emulate this code structure I get

>>> class Foo:
...     def __init__(self):
...         baz = 42
...     @property
...     def bar(self):
...         return self.baz
... 
>>> f=Foo()
>>> f.bar()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 6, in bar
AttributeError: 'Foo' object has no attribute 'baz'. Did you mean: 'bar'?
>>> f.baz
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'Foo' object has no attribute 'baz'. Did you mean: 'bar'?

It's all more or less how it's documented in https://docs.python.org/3/library/functions.html#property, and I don't understand why it doesn't work for me. However, the latter docs seem to indicate that what's inside __init__ is not directly accessible in a class instance, you'd need to get a "getter" for it.


Edit: I should have written self.baz = 42 above, then it would have worked. But anyway...
There is also some stuff in https://docs.python.org/3/tutorial/classes.html#private-variables - which gives me a headache...

@dimpase
Copy link
Member

dimpase commented Mar 11, 2025

By the way, here is a naive attempt to reproduce this with sphinx 8.2.3, but to no avail:

$ python
Python 3.12.9 (main, Feb 24 2025, 04:00:05) [GCC 14.2.1 20241221] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from sphinx import registry
>>> registry.SphinxComponentRegistry
<class 'sphinx.registry.SphinxComponentRegistry'>
>>> t=registry.SphinxComponentRegistry()
>>> type(t)
<class 'sphinx.registry.SphinxComponentRegistry'>
>>> t.autodoc_attrgettrs
{}
>>> t.autodoc_attrgetters
{}

@kiwifb
Copy link
Member Author

kiwifb commented Mar 11, 2025

Experimenting with sphinx 8.2.3. There are numerous errors and touching attrgettrs does not solves them. sage_autodoc.py probably needs a new rebase to work with sphinx 8.2.3 properly. I am a bit overcommitted at the moment so it would be helpful if someone else volunteered for the job. I cannot promise anything before next week - and I'd need a reminder.

@dimpase
Copy link
Member

dimpase commented Mar 11, 2025

Experimenting with sphinx 8.2.3. There are numerous errors and touching attrgettrs does not solves them. sage_autodoc.py probably needs a new rebase to work with sphinx 8.2.3 properly. I am a bit overcommitted at the moment so it would be helpful if someone else volunteered for the job. I cannot promise anything before next week - and I'd need a reminder.

It worked for me with Python 3.12. Did you try with 3.13?

@kiwifb
Copy link
Member Author

kiwifb commented Mar 11, 2025

Why it used python 3.12 instead of 3.13 is also on my TODO list of things to look at.

@kiwifb
Copy link
Member Author

kiwifb commented Mar 11, 2025

Why it used python 3.12 instead of 3.13 is also on my TODO list of things to look at.

I actually figured that one out, gentoo's ebuilds for sphinx-copybutton and sphinx-inline-tabs do not support python 3.13 yet. They block doc building with 3.13.

@jhpalmieri
Copy link
Member

jhpalmieri commented Mar 12, 2025

I think this broke something with Sage. I see this in the Sphinx log file:

[spkg-pipinst] ERROR: Could not find a version that satisfies the requirement sphinxcontrib-htmlhelp>=2.0.6 (from sphinx) (from versions: 2.0.5)
[spkg-pipinst] ERROR: No matching distribution found for sphinxcontrib-htmlhelp>=2.0.6

...

[spkg-pipinst] Successfully installed sphinx-8.1.3
[spkg-pipinst] Warning: The installation needed to use "--no-deps --ignore-installed --ignore-requires-python" to succeed. This means that a dependencies file in build/pkgs/ needs to be updated. Please report this to [email protected], including the build log of this package.

My guess is that sphinxcontrib-htmlhelp needs a version bump, too. Maybe some other components of Sphinx, in addition? This leads to Sage's Python to fail (some of the time, I don't know why not all of the time), and this leads to a failure of Sage to detect the correct number of CPUs to use for parallel processing. See https://groups.google.com/g/sage-devel/c/lr03GlRBr9g/m/Yx8yo7p4AQAJ.

@kiwifb
Copy link
Member Author

kiwifb commented Mar 12, 2025

I definitely overlooked bumping those.

@dimpase
Copy link
Member

dimpase commented Mar 12, 2025

Building on my Gentoo machine worked (modulo the fix above) as my sphinx and its deps come from the OS in my regular builds, and are quite new

@kiwifb kiwifb mentioned this pull request Mar 12, 2025
5 tasks
@kiwifb
Copy link
Member Author

kiwifb commented Mar 12, 2025

I think this broke something with Sage. I see this in the Sphinx log file:

[spkg-pipinst] ERROR: Could not find a version that satisfies the requirement sphinxcontrib-htmlhelp>=2.0.6 (from sphinx) (from versions: 2.0.5)
[spkg-pipinst] ERROR: No matching distribution found for sphinxcontrib-htmlhelp>=2.0.6

...

[spkg-pipinst] Successfully installed sphinx-8.1.3
[spkg-pipinst] Warning: The installation needed to use "--no-deps --ignore-installed --ignore-requires-python" to succeed. This means that a dependencies file in build/pkgs/ needs to be updated. Please report this to [email protected], including the build log of this package.

My guess is that sphinxcontrib-htmlhelp needs a version bump, too. Maybe some other components of Sphinx, in addition? This leads to Sage's Python to fail (some of the time, I don't know why not all of the time), and this leads to a failure of Sage to detect the correct number of CPUs to use for parallel processing. See https://groups.google.com/g/sage-devel/c/lr03GlRBr9g/m/Yx8yo7p4AQAJ.

Can you see if #39686 helps you and review it please.

@kiwifb
Copy link
Member Author

kiwifb commented Mar 18, 2025

@kiwifb with sphinx 8.2.3 I had to apply

--- a/src/sage_docbuild/ext/sage_autodoc.py
+++ b/src/sage_docbuild/ext/sage_autodoc.py
@@ -2976,7 +2976,7 @@ class PropertyDocumenter(DocstringStripSignatureMixin,  # type: ignore[misc]
 
 def autodoc_attrgetter(app: Sphinx, obj: Any, name: str, *defargs: Any) -> Any:
     """Alternative getattr() for types"""
-    for typ, func in app.registry.autodoc_attrgetters.items():
+    for typ, func in app.registry.autodoc_attrgettrs.items():
         if isinstance(obj, typ):
             return func(obj, name, *defargs)
 

Was it renamed in 8.2, or it's a typo in the branch?

I have been unable to build the doc with sphinx 8.2.3 so far. Even after updating sage_autodoc.py (on hold for now). The problem I have is completely unrelated

Versions
========

* Platform:         linux; (Linux-6.12.16-gentoo-dist-x86_64-Intel-R-_Core-TM-_i7-9700_CPU_@_3.00GHz-with-glibc2.40)
* Python version:   3.12.9 (CPython)
* Sphinx version:   8.2.3
* Docutils version: 0.21.2
* Jinja2 version:   3.1.5
* Pygments version: 2.19.1

Last Messages
=============

    gast


    writing output... [ 24%]
    gc


    writing output... [ 24%]
    gcc

Loaded Extensions
=================

* sphinx.ext.mathjax (8.2.3)
* alabaster (1.0.0)
* sphinxcontrib.applehelp (2.0.0)
* sphinxcontrib.devhelp (2.0.0)
* sphinxcontrib.htmlhelp (2.1.0)
* sphinxcontrib.serializinghtml (2.0.0)
* sphinxcontrib.qthelp (2.0.0)
* sage_docbuild.ext.inventory_builder (unknown version)
* sage_docbuild.ext.multidocs (unknown version)
* sphinx.ext.autodoc.preserve_defaults (8.2.3)
* sphinx.ext.autodoc.type_comment (8.2.3)
* sphinx.ext.autodoc.typehints (8.2.3)
* sage_docbuild.ext.sage_autodoc (8.2.3)
* sphinx.ext.todo (8.2.3)
* sphinx.ext.extlinks (8.2.3)
* sphinx.ext.linkcode (8.2.3)
* sphinx_copybutton (0.5.2)
* sphinx_inline_tabs (2023.04.21)
* IPython.sphinxext.ipython_directive (unknown version)
* matplotlib.sphinxext.plot_directive (3.10.1)
* jupyter_sphinx (0.5.3)

Traceback
=========

    Traceback (most recent call last):
      File "/usr/lib/python3.12/site-packages/sphinx/events.py", line 404, in emit
        results.append(listener.handler(self.app, *args))
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib/python3.12/site-packages/sage_docbuild/conf.py", line 783, in find_sage_dangling_links
        res = call_intersphinx(app, env, node, contnode)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib/python3.12/site-packages/sage_docbuild/conf.py", line 746, in call_intersphinx
        res = intersphinx.missing_reference(
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib/python3.12/site-packages/sphinx/ext/intersphinx/_resolve.py", line 343, in missing_reference
        return resolve_reference_detect_inventory(env, node, contnode)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib/python3.12/site-packages/sphinx/ext/intersphinx/_resolve.py", line 311, in resolve_reference_detect_inventory
        resolve_self = env.config.intersphinx_resolve_self
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib/python3.12/site-packages/sphinx/config.py", line 497, in __getattr__
        raise AttributeError(msg)
    AttributeError: No such config value: 'intersphinx_resolve_self'
    
    The above exception was the direct cause of the following exception:
    
    Traceback (most recent call last):
      File "/usr/lib/python3.12/site-packages/sphinx/cmd/build.py", line 432, in build_main
        app.build(args.force_all, args.filenames)
      File "/usr/lib/python3.12/site-packages/sphinx/application.py", line 426, in build
        self.builder.build_update()
      File "/usr/lib/python3.12/site-packages/sphinx/builders/__init__.py", line 375, in build_update
        self.build(
      File "/usr/lib/python3.12/site-packages/sphinx/builders/__init__.py", line 454, in build
        self.write(docnames, updated_docnames, method)
      File "/usr/lib/python3.12/site-packages/sphinx/builders/__init__.py", line 735, in write
        self.write_documents(docnames)
      File "/usr/lib/python3.12/site-packages/sphinx/builders/__init__.py", line 749, in write_documents
        self._write_serial(sorted_docnames)
      File "/usr/lib/python3.12/site-packages/sphinx/builders/__init__.py", line 765, in _write_serial
        doctree = self.env.get_and_resolve_doctree(docname, self)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib/python3.12/site-packages/sphinx/environment/__init__.py", line 701, in get_and_resolve_doctree
        self.apply_post_transforms(doctree, docname)
      File "/usr/lib/python3.12/site-packages/sphinx/environment/__init__.py", line 773, in apply_post_transforms
        transformer.apply_transforms()
      File "/usr/lib/python3.12/site-packages/sphinx/transforms/__init__.py", line 92, in apply_transforms
        super().apply_transforms()  # type: ignore[misc]
        ^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib/python3.12/site-packages/docutils/transforms/__init__.py", line 182, in apply_transforms
        transform.apply(**kwargs)
      File "/usr/lib/python3.12/site-packages/sphinx/transforms/post_transforms/__init__.py", line 46, in apply
        self.run(**kwargs)
      File "/usr/lib/python3.12/site-packages/sphinx/transforms/post_transforms/__init__.py", line 75, in run
        new_node = self._resolve_pending_xref(node, contnode)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib/python3.12/site-packages/sphinx/transforms/post_transforms/__init__.py", line 128, in _resolve_pending_xref
        new_node = self.app.events.emit_firstresult(
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib/python3.12/site-packages/sphinx/events.py", line 433, in emit_firstresult
        for result in self.emit(name, *args, allowed_exceptions=allowed_exceptions):
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib/python3.12/site-packages/sphinx/events.py", line 415, in emit
        raise ExtensionError(
    sphinx.errors.ExtensionError: Handler <function find_sage_dangling_links at 0x7f1c9c175d00> for event 'missing-reference' threw an exception (exception: No such config value: 'intersphinx_resolve_self')

Which basically complains that sage_docbuild/conf.py doesn't set intersphinx_resolve_self which from the documentation (https://www.sphinx-doc.org/en/master/usage/extensions/intersphinx.html) is blank by default and was never an issue before. Did you do anything else to be able to build?

@kiwifb
Copy link
Member Author

kiwifb commented Mar 18, 2025

OK, I have a rebase on sphinx 8.2.3 with a working update to sage_docbuild/conf.py but it is not backward compatible with sphinx 8.1.3, so it will also include a bump to the latest sphinx. I'll get the PR ready in a few hours. Anyone has an issue with bumping sphinx again now?

vbraun pushed a commit to vbraun/sage that referenced this pull request Mar 19, 2025
sagemathgh-39686: update sphinxcontrib packages
    
This is a follow up to sagemath#39577 where sphinx was update but not the
sphinxcontrib packages.
This causes breakages for some users.



### 📝 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

sagemath#39680 deals with one the packages that would be covered by this PR
<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39686
Reported by: François Bissey
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Mar 22, 2025
sagemathgh-39686: update sphinxcontrib packages
    
This is a follow up to sagemath#39577 where sphinx was update but not the
sphinxcontrib packages.
This causes breakages for some users.



### 📝 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

sagemath#39680 deals with one the packages that would be covered by this PR
<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39686
Reported by: François Bissey
Reviewer(s):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants