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

Meson: compile without embed-positions #39745

Closed
wants to merge 1 commit into from

Conversation

tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Mar 20, 2025

Fixes #39735.

📝 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

@user202729
Copy link
Contributor

user202729 commented Mar 20, 2025

This is obviously a bad idea (for one when you type function?? into the command-line for certain functions you will no longer be able to view the source code, I think). Rather you should probably modify the code somehow to gracefully fail when the source code cannot be found.

Side note: Assume #39279 is merged, I guess.

Copy link

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

@tobiasdiez
Copy link
Contributor Author

This is obviously a bad idea

I'm still trying to understand what this option actually does (or better: where the file path is actually used in sage). Since you worked on this recently, can you please expand why you think it is a bad idea.

(for one when you type function?? into the command-line for certain functions you will no longer be able to view the source code, I think). Rather you should probably modify the code somehow to gracefully fail when the source code cannot be found.

Since the pyx files are not distributed in the wheel, this info wouldn't be available anyway in a "normal" installation. It only makes sense for editable installs, where one could probably fall-back to SAGE_SRC anyway.

@user202729
Copy link
Contributor

user202729 commented Mar 20, 2025

expand why you think it is a bad idea

Looks like you already got the point, that ?? depends on this feature because some ipython hook modifies the imports in ipython to point to functions in sageinspect which parses this docstring.

p/s, no offense intended. Sorry if it came across as such.

Since the pyx files are not distributed in the wheel, this info wouldn't be available anyway in a "normal" installation

Yes I got that, but removing the embedding would also break ?? in cases where pyx files are available right?

@tornaria
Copy link
Contributor

Could we just ship the .pyx files (and make sure there are no absolute paths)? We definitely want ?? working in any case, don't we?

@tobiasdiez
Copy link
Contributor Author

expand why you think it is a bad idea

Looks like you already got the point, that ?? depends on this feature because some ipython hook modifies the imports in ipython to point to functions in sageinspect which parses this docstring.

p/s, no offense intended. Sorry if it came across as such.

No offense taken! Just wanted to make sure that only the feature ?? is affected.

Since the pyx files are not distributed in the wheel, this info wouldn't be available anyway in a "normal" installation

Yes I got that, but removing the embedding would also break ?? in cases where pyx files are available right?

I thought that your recent PR for the special handling of the editable meson install would work here (obviously, the broken CI proofs that my assumption is wrong). So the idea would be: if it's an editable install, then use the module name relative to SAGE_SRC as the source file. Would that work?

Could we just ship the .pyx files (and make sure there are no absolute paths)? We definitely want ?? working in any case, don't we?

I just tried numpy.random.random_sample?? and it doesn't show any source code info (while numpy.amax?? does). So even if ?? doesn't work for cython method, it at least wouldn't be a worse user experience than for numpy.
But yes, probably we could ship .pyx and use importlib to find them.

@tornaria
Copy link
Contributor

Could we just ship the .pyx files (and make sure there are no absolute paths)? We definitely want ?? working in any case, don't we?

I just tried numpy.random.random_sample?? and it doesn't show any source code info (while numpy.amax?? does). So even if ?? doesn't work for cython method, it at least wouldn't be a worse user experience than for numpy. But yes, probably we could ship .pyx and use importlib to find them.

I'm talking about sage. Things like:

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):
[...]

@user202729
Copy link
Contributor

user202729 commented Mar 24, 2025

Just wanted to make sure that only the feature ?? is affected

More like it's the only feature that I have come across that make use of this. There might be more that I'm not aware of.

I just tried numpy.random.random_sample?? and it doesn't show any source code info (while numpy.amax?? does).

The problem is because numpy does not embed the signature or the source location in its docstring, so we have no choice.

https://github.com/numpy/numpy/blob/main/numpy/random/mtrand.pyx#L389

sage: print(np.random.random_sample.__doc__)

        random_sample(size=None)

        Return random floats in the half-open interval [0.0, 1.0).

Compare:

sage: print(prime_pi.__call__.__doc__)
File: /…/sage/src/sage/functions/prime_pi.pyx (starting at line 91)

        EXAMPLES::

            sage: # needs sage.symbolic

By the way, #39279 will show more information (signature and call signature) in ??.

@tobiasdiez
Copy link
Contributor Author

Okay, let's close it for now. Once mesonbuild/meson-python#256 is implemented, we could disable the embedding of the filename for non-editable installs.

@tobiasdiez tobiasdiez closed this Mar 28, 2025
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.

Meson build: cached methods are broken if the source is unavailable
3 participants