Many more namespace packages#35322
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #35322 +/- ##
===========================================
+ Coverage 88.62% 88.65% +0.03%
===========================================
Files 2148 2114 -34
Lines 398855 398679 -176
===========================================
- Hits 353480 353462 -18
+ Misses 45375 45217 -158
... and 62 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
@tornaria There is only one doctest failure, for I don't know if we care much about this function, but we can repair it using our function |
I think you need to
The following fixes it for me: diff --git a/src/sage/misc/package_dir.py b/src/sage/misc/package_dir.py
index bb4e34d5c26..d4e5cb3bc7a 100644
--- a/src/sage/misc/package_dir.py
+++ b/src/sage/misc/package_dir.py
@@ -155,7 +155,7 @@ def is_package_or_sage_namespace_package_dir(path, *, distribution_filter=None):
:mod:`sage.cpython` is an ordinary package::
sage: from sage.misc.package_dir import is_package_or_sage_namespace_package_dir
- sage: directory = os.path.dirname(sage.cpython.__file__); directory
+ sage: directory = sage.cpython.__path__[0]; directory
'.../sage/cpython'
sage: is_package_or_sage_namespace_package_dir(directory)
True
@@ -163,21 +163,21 @@ def is_package_or_sage_namespace_package_dir(path, *, distribution_filter=None):
:mod:`sage.libs.mpfr` only has an ``__init__.pxd`` file, but we consider
it a package directory for consistency with Cython::
- sage: directory = os.path.join(os.path.dirname(sage.libs.all.__file__), 'mpfr'); directory
+ sage: directory = os.path.join(sage.libs.__path__[0], 'mpfr'); directory
'.../sage/libs/mpfr'
sage: is_package_or_sage_namespace_package_dir(directory)
True
:mod:`sage` is designated to become an implicit namespace package::
- sage: directory = os.path.dirname(sage.env.__file__); directory
+ sage: directory = sage.__path__[0]; directory
'.../sage'
sage: is_package_or_sage_namespace_package_dir(directory)
True
Not a package::
- sage: directory = os.path.join(os.path.dirname(sage.symbolic.__file__), 'ginac'); directory
+ sage: directory = os.path.join(sage.symbolic.__path__[0], 'ginac'); directory
'.../sage/symbolic/ginac'
sage: is_package_or_sage_namespace_package_dir(directory)
False
|
|
BTW, I noticed that the function |
I think only this last example needs to be adjusted; the preceding examples are modules or ordinary packages. |
OK, this is from an unpatched Cython without namespace package support. |
Hm, no, that is already used in |
|
I can't reproduce this failure here, also not with stock Cython 0.29.33 installed via pip |
I think the first use of that is on one of the pending tickets from #29705. I'll check later |
Yes, but using |
I'll have a look and patch our Cython if it's necessary. |
Did you test with Maybe |
Oh... now I can reproduce it, thanks... |
I patched cython with https://github.com/sagemath/sage/blob/develop/build/pkgs/cython/patches/4918.patch, but I'm still getting the same failure. Either with 10.0.beta5 + 35322, or just with my system sagemath 9.8 when I remove the empty |
|
Also, note that works, but doesn't (same failure as Is the second one supposed to work? Both that one and the original failure work ok when In the same style, should |
It would be worth checking what Cython 3 thinks about this. |
|
Summary:
Here is a short term workaround: --- a/src/sage/symbolic/pynac.pxi
+++ b/src/sage/symbolic/pynac.pxi
@@ -3,9 +3,9 @@ Declarations for pynac, a Python frontend for ginac
Check that we can externally cimport this (:trac:`18825`)::
- sage: cython( # long time; random compiler warnings # optional - sage.misc.cython
+ sage: cython( # optional - sage.misc.cython
....: '''
- ....: from sage.symbolic cimport expression
+ ....: cimport sage.symbolic.expression
....: ''')
"""
I removed the # long time and random since I don't think it's necessary, you may know better... The long term solution is for cython to fix its pep 420 support. Meanwhile, it would be nice to add the following tests: ---- a/src/sage/misc/cython.py
+++ b/src/sage/misc/cython.py
@@ -211,6 +211,28 @@ def cython(filename, verbose=0, compile_message=False,
sage: cython('''
....: cdef size_t foo = 3/2
....: ''')
+
+ Check that Cython supports PEP 420 packages::
+
+ sage: cython('''
+ ....: cimport sage.misc.cachefunc
+ ....: ''')
+
+ sage: cython('''
+ ....: from sage.misc.cachefunc cimport cache_key
+ ....: ''')
+
+ In Cython 0.29.33 using `from PACKAGE cimport MODULE` is broken
+ when `PACKAGE` is a namespace package, see :trac:`35322`::
+
+ sage: cython('''
+ ....: from sage.misc cimport cachefunc
+ ....: ''')
+ Traceback (most recent call last):
+ ...
+ RuntimeError: Error compiling Cython file:
+ ...
+ ...: 'sage/misc.pxd' not found
"""
if not filename.endswith('pyx'):
print("Warning: file (={}) should have extension .pyx".format(filename), file=sys.stderr)The last one is the broken one, this will warn us when it gets fixed... |
|
Also, just |
Still broken in the same way as in 0.29.33. I'll submit an issue. What is broken: What works ok: [1] is [1] either with Let's just move on, if you are ok with my suggestions above. |
|
Excellent, thanks very much. Yes, I agree with your fixes; I'll put them on the branch. |
You didn't respond to this. I still think using Do you see any drawback to changing all the tests as I suggested? |
|
OK, I will push these changes too |
…namespacification
Thanks, I'll clean up my worktree and test the PR as it is now, just to double check everything is ok, then approve since everything looks good now. |
|
For the record, list of empty What is the criteria to keep these? |
|
Documentation preview for this PR is ready! 🎉 |
|
Everything works ok for me on dc8b3c5. I'm approving, although I'm still wondering about the 29 empty In addition, among the 25 nonempty
|
|
About my original concern of package conflicts between system installed sagemath vs. user installed one, I propose the following approach:
This ensures that we avoid conflicts with system packages. Anything having an empty |
|
Thanks! Sorry for the slowness in responding to your comments.
I kept these because they looked "self-contained" to me, but it's fine with me to remove them.
Quite likely, each of these can remain an ordinary package; we can look into this in more detail in a follow up.
All of
Kept these because I don't have a plan for what to do with sage.tests yet. Likely to remain monolithic.
OK, we can take care of the docstring using
Follow-up PR
Monolithic
Monolithic by design |
Sounds good to me. PR to add this to |
tornaria
left a comment
tornaria
left a comment
There was a problem hiding this comment.
I'm removing my approval for this until all the troublesome interaction between system sagemath and development sagemath is sorted out.
|
@tornaria, the idea of such a configuration -- old incompatible Sage installed in system package, and hoping that the development Sage shadows it properly -- is just fundamentally flawed and cannot be supported. It's not just namespace packages vs old ordinary packages. Also when a Python module in the new version replaces a Cython module of the same name (as happened recently with |
|
As I wrote to your privately, if you need this for some reason (likely because your platform or workflow is not compatible with / aware of venvs), just putting an empty file |
I will test this and report back. The statement that there is no way to support a (quite up to date) system package of sagemath simultaneously with a development version of sagemath where I don't have to build infinite stuff other than the sagelib itself, it's very discouraging. I'm willing to change my workflow, as long as it doesn't mean using a local "venv" in which I have to replicate all python packages that I already have installed in my system. It seems your idea will solve that. Seriously, my workflow is very, very, lean, and it's a world of difference compared to anything else I knew before, that I'd rather stop using sage than go back to |
|
I'm willing to change my workflow, as long as it doesn't mean using a
local "venv" in which I have to replicate all python packages that I
already have installed in my system. It seems your idea will solve that.
I think distro package maintainers of Python packages should look into
providing PEP-660 "editable wheels" that correspond to packages installed
in system site-packages. (They wouldn't be really editable because it would
point to a root-owned location.) It's basically a mechanism to install a
symlink farm using Python packaging infrastructure.
This would allow users to provision venvs that contain selected
distribution-provided Python packages (instead of installing them from pip).
|
|
The solution of creating an empty I've tested both this PR and #35366 in this way, with sagemath 9.8 installed from system, without having to adjust anything in the system package. Thus, I'm giving back my positive review / approval for this PR, which I had already looked at. Thanks for your patience. |
|
Glad that this is working for you! Thanks for the review. |
gh-35366: Many more namespace packages – follow up <!-- Please provide a concise, informative and self-explanatory title. --> <!-- Don't put issue numbers in the title. Put it in the Description below. --> <!-- For example, instead of "Fixes #12345", use "Add a new method to multiply two integers" --> ### 📚 Description <!-- Describe your changes here in detail. --> Follow-up from #35322. <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes #12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x ]`. --> - [x] The title is concise, informative, and self-explanatory. - [ ] 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 accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - #12345: short description why this is a dependency - #34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: #35366 Reported by: Matthias Köppe Reviewer(s): Gonzalo Tornaría
gh-35372: Replace more `.all` imports <!-- Please provide a concise, informative and self-explanatory title. --> <!-- Don't put issue numbers in the title. Put it in the Description below. --> <!-- For example, instead of "Fixes #12345", use "Add a new method to multiply two integers" --> ### 📚 Description This is a follow-up on: - #35110 As preparation for #35322, which is changing more packages to implicit namespace packages, we remove `.all` imports from these packages throughout the Sage library. This is part of: - #29705 <!-- Describe your changes here in detail. --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes #12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x ]`. --> - [x] The title is concise, informative, and self-explanatory. - [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 accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - #12345: short description why this is a dependency - #34567: ... --> - Depends on #35418 - Depends on #35358 <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: #35372 Reported by: Matthias Köppe Reviewer(s): Gonzalo Tornaría
4 participants
📚 Description
As discussed in #35100 (comment), we remove many of the empty
__init__.pyfiles, turning these packages into PEP-420 implicit namespace packages.The following packages will keep their
__init__.pyfiles because they are intended to remain ordinary packages indefinitely:sage.cpython,sage.structure– because they are shipped as a whole by sagemath-objectssage.doctest,sage.repl– because they are shipped as a whole by sagemath-replsage.features– because it is shipped as a whole by sagemath-environmentThis is part of:
📝 Checklist
⌛ Dependencies