-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
Many more namespace packages #35322
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. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
<!-- 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
<!-- 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
📚 Description
As discussed in #35100 (comment), we remove many of the empty
__init__.py
files, turning these packages into PEP-420 implicit namespace packages.The following packages will keep their
__init__.py
files 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