Skip to content

Show signature for binding=False cython functions #39279

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

Merged
merged 13 commits into from
Apr 29, 2025

Conversation

user202729
Copy link
Contributor

@user202729 user202729 commented Jan 4, 2025

Fixes #26254 , without binding=True. (That is, this shows the signature correctly in function? for certain functions compiled in Cython.)

Before:

sage: prime_pi??
Type:           PrimePi
String form:    prime_pi
File:           /usr/lib/python3.13/site-packages/sage/functions/prime_pi.pyx
Source:        
cdef class PrimePi(BuiltinFunction):
[...]

After:

sage: prime_pi??
Signature:      prime_pi()
Call signature: prime_pi(self, coerce=True, hold=False, *args)
Type:           PrimePi
String form:    prime_pi
File:           ~/sage/src/sage/functions/prime_pi.pyx
Source:        
cdef class PrimePi(BuiltinFunction):

Even if we ultimately decide to set binding=True, this would probably still be desirable, in case some other library chooses to use binding=False, embedsignature=True.

Note: there's an alternative of implementing this into IPython, see ipython/ipython#14639

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

sage: def f(x, y, z=1, t=2, *args, **keywords):
....: pass
sage: sage_signature(f)
<Signature (x, y, z=1, t=2, *args, **keywords)>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

examples mostly copied from sage_getargspec, modified accordingly.

Copy link

github-actions bot commented Jan 5, 2025

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

@tobiasdiez
Copy link
Contributor

Note: there's an alternative of implementing this into IPython

Would this be much work? Since this seems to be 'just' a cython-specific problem, and nothing intrinsically related to sage, I would prefer if this could be fixed upstream in IPython (in fact, I hope that most of the special Cython handling in sage.inspect could be moved to upstream, either IPython or Python)

@user202729
Copy link
Contributor Author

Might be worth trying, let's see.

@user202729
Copy link
Contributor Author

(at the moment I don't really have time to port the change to IPython, nor to deal with the various versioning issue e.g. if ipython version ≥ X implements this, then sage need to be sure to not worsen the situation by monkey patching its own function into ipython if version is ≥ X. On the other hand it's comparatively easier to implement this directly into sage since the framework is already there. We can just get it in first then do the refactor later, which also has the added benefit of not having to wait until the next ipython release to have the feature available?)

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.

Okay, let's get it in; fixing this upstream and converting to binding=true in sage is still desirable in my opinion.

@vbraun
Copy link
Member

vbraun commented Apr 13, 2025

PDF docbuild fails

[692] [693] [694] [695] [696] [697] [698]
! Missing { inserted.
<to be read again> 
_
l.63694 \(__
          new__\), \(__init__\) or \(__call__\) method is returned:

@user202729
Copy link
Contributor Author

user202729 commented Apr 13, 2025

Ah, looks like it failed on CI too. I guess the CI is not reliable enough for me to notice it…

2025-03-27T09:21:00.3143798Z [sagemath_doc_pdf-none] [spkg-install] Collected error summary (may duplicate other messages):
2025-03-27T09:21:00.3144200Z [sagemath_doc_pdf-none] [spkg-install]   pdflatex: Command for 'pdflatex' gave return code 1
2025-03-27T09:21:00.3144602Z [sagemath_doc_pdf-none] [spkg-install]       Refer to 'misc.log' and/or above output for details

[trying to diagnose the issue]

it's using batchmode which hides the issue… (src/doc/Makefile → doc-pdf)

(also confusingly it's named .zip even though it's gzip compressed)

[scroll up]

LATEXOPTS="--file-line-error --interaction=batchmode" sage --docbuild reference/misc pdf --no-prune-empty-dirs

so I try

cd pkgs/sage-docbuild/
pip install .
cd ../..
cd src/doc/en/reference
pip install sphinx-copybutton   # why is this necessary? shouldn't it be pulled from the above?
LATEXOPTS="--file-line-error" sage --docbuild reference/misc pdf --no-prune-empty-dirs

it's compiling.

LaTeX files can be found in /home/user202729/miniforge3/envs/sage-dev/share/doc/sage/latex/en/reference/misc.

I look at the misc.tex file inside there and see

\sphinxAtStartPar
In the case of a class or a class instance, the \sphinxcode{\sphinxupquote{FullArgSpec}} of the
\sphinxcode{\sphinxupquote{\_\_new\_\_}}, \sphinxcode{\sphinxupquote{\_\_init\_\_}} or \sphinxcode{\sphinxupquote{\_\_call\_\_}} method is returned:

Looks like the same line. In develop branch on my side it's properly escaped. But in this branch it is

\sphinxAtStartPar
In the case of a class or a class instance, the \(Signature\) of the
\(__new__\), \(__init__\) or \(__call__\) method is returned:

Why does the issue happen…


I think building docs sort of mostly work in meson, would be nice (and save electricity) if that can be converted to meson too… easier said than done though.

@user202729
Copy link
Contributor Author

Should be fixed now, I believe.

Lesson to be learnt when AI output is not checked properly I suppose…

@user202729
Copy link
Contributor Author

@tobiasdiez Can you review again, thanks. (actually previously the PDF build failure is also reflected by the formulas not rendering correctly on the documentation preview, both works now including the documentation preview)

vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 20, 2025
sagemathgh-39939: Add sphinx-copybutton to dependency of sage-docbuild
    
Reason: the package doesn't run correctly without the dependency. It is
listed as a dependency in the sage spkg too
(`build/pkgs/sagemath_doc_html/dependencies`)

First noticed while working on
sagemath#39279 .

### 📝 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.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] 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: ... -->
    
URL: sagemath#39939
Reported by: user202729
Reviewer(s): Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 21, 2025
sagemathgh-39939: Add sphinx-copybutton to dependency of sage-docbuild
    
Reason: the package doesn't run correctly without the dependency. It is
listed as a dependency in the sage spkg too
(`build/pkgs/sagemath_doc_html/dependencies`)

First noticed while working on
sagemath#39279 .

### 📝 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.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] 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: ... -->
    
URL: sagemath#39939
Reported by: user202729
Reviewer(s): Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 29, 2025
sagemathgh-39939: Add sphinx-copybutton to dependency of sage-docbuild
    
Reason: the package doesn't run correctly without the dependency. It is
listed as a dependency in the sage spkg too
(`build/pkgs/sagemath_doc_html/dependencies`)

First noticed while working on
sagemath#39279 .

### 📝 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.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] 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: ... -->
    
URL: sagemath#39939
Reported by: user202729
Reviewer(s): Tobias Diez
@vbraun vbraun merged commit c3a4aa6 into sagemath:develop Apr 29, 2025
21 checks passed
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.

Use Cython directive binding=True to get signatures for cython methods
3 participants