New ABC sage.structure.element.NumberFieldElement, deprecate is_NumberFieldElement

[mkoeppe]

📚 Description

Fixes #34931

📝 Checklist

• I have made sure that the title is self-explanatory and the description concisely explains the PR.
• I have linked an issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation accordingly.

⌛ Dependencies

Depends on #35033

[codecov-commenter]

Codecov Report

Patch coverage: 98.31% and project coverage change: +0.01 🎉

Comparison is base (52a81cb) 88.57% compared to head (0c894e0) 88.58%.

❗ Current head 0c894e0 differs from pull request most recent head 57fcada. Consider uploading reports for the commit 57fcada to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #35100      +/-   ##
===========================================
+ Coverage    88.57%   88.58%   +0.01%     
===========================================
  Files         2140     2140              
  Lines       397273   397108     -165     
===========================================
- Hits        351891   351793      -98     
+ Misses       45382    45315      -67     

Impacted Files	Coverage Δ
src/sage/symbolic/expression_conversions.py	91.06% <55.55%> (-0.29%)	⬇️
src/sage/interfaces/maxima_lib.py	93.78% <100.00%> (+0.04%)	⬆️
src/sage/rings/universal_cyclotomic_field.py	97.77% <100.00%> (ø)
src/sage/schemes/affine/affine_morphism.py	90.33% <100.00%> (ø)
src/sage/schemes/elliptic_curves/BSD.py	43.75% <100.00%> (+0.21%)	⬆️
src/sage/schemes/elliptic_curves/cardinality.py	87.54% <100.00%> (+0.93%)	⬆️
src/sage/schemes/elliptic_curves/cm.py	89.52% <100.00%> (+0.39%)	⬆️
.../sage/schemes/elliptic_curves/ell_curve_isogeny.py	94.92% <100.00%> (ø)
src/sage/schemes/elliptic_curves/ell_field.py	80.92% <100.00%> (ø)
...c/sage/schemes/elliptic_curves/ell_finite_field.py	87.92% <100.00%> (-0.92%)	⬇️
... and 88 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mkoeppe]

By the way, did you see #34931 (comment)?

[tscrim]

Thank you. LGTM.

[tscrim]

By the way, did you see #34931 (comment)?

I did, but I don't know which is better either. At this point, I think it is best to keep with the same pattern as it makes it easier to refactor stuff later on if something changes.

[mkoeppe]

same pattern as what?

[tscrim]

same pattern as what?

As the other ABCEDFITs by being in element.pxd/pyx.

[mkoeppe]

Note that in #35076 (for CommutativePolynomial etc.), we moved away from that again...

[tscrim]

I forgot that. Hmm...then perhaps we should put it into its own little file. I am worried a bit about bloat within element.p*. It does make a bit more sense within element.pyx if it was a true ABC that people could subclass of off it. Although since it is not meant for that, it could easily get confused.

[mkoeppe]

OK, then I'll make this change.

[tscrim]

Thanks. LGTM.

[github-actions]

Documentation preview for this PR is ready! 🎉
Built with commit: 0c894e0

[tornaria]

@mkoeppe : for some reason I don't understand, the removal of .../number_field/__init__.py causes number_field_element_base to be unimportable:

$ PYTHONPATH=pkgs/sagemath-standard/build/lib.linux-x86_64-cpython-311 python
Python 3.11.2 (main, Feb  8 2023, 14:30:35) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sage.rings.number_field.number_field_element_base
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'sage.rings.number_field.number_field_element_base'
>>> 
$ touch pkgs/sagemath-standard/build/lib.linux-x86_64-cpython-311/sage/rings/number_field/__init__.py
$ PYTHONPATH=pkgs/sagemath-standard/build/lib.linux-x86_64-cpython-311 python
Python 3.11.2 (main, Feb  8 2023, 14:30:35) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sage.rings.number_field.number_field_element_base
>>> 

I'm doing it here with plain python, but the same thing happens with sage.

There are lots of empty __init__.py files:

$ find -name __init__.py  -size 0 | wc -l
327

[tornaria]

This is what I think happens:

• I have sage installed system-wide
• Hence there is an (empty) file /usr/lib/python3.11/site-packages/sage/rings/number_field/__init__.py
• When doing import sage.rings.number_field.number_field_element_base, python tries first to find the module sage.rings.number_field
• Since PYTHONPATH=pkgs/sagemath-standard/build/lib.linux-x86_64-cpython-311, it looks there for this module. Without the empty __init__.py there, this fails.
• Python still wants sage.rings.number_field, so it keeps looking and finds it at the system site-packages.
• Now the submodule number_field_element_base is searched only at site-packages but my system install is 9.8 so it doesn't contain it, hence the failure.

If I'm correct, this explains why only number_field_element_base fails to import. But on the other hand it means that importing other submodules of number_field will pick them from the site-packages and not from the current location.

In other words, a system install of sagemath will "shadow" a user install; in principle this shouldn't be an issue, except when the user install is missing a module python will pick it from the system install. The only way to avoid this seems to be to keep empty modules to 'hide" the corresponding system modules.

[mkoeppe]

Yes, ordinary packages shadow namespace packages. A configuration in which an old version of Sage is installed in system site-packages cannot be reliably supported.

[tornaria]

According to https://docs.python.org/3/tutorial/modules.html#packages:

The __init__.py files are required to make Python treat directories containing the file as packages. This prevents directories with a common name, such as string, from unintentionally hiding valid modules that occur later on the module search path. In the simplest case, __init__.py can just be an empty file, but it can also execute initialization code for the package or set the __all__ variable, described later.

Since sage.rings.number_field is a package (it contains modules) the __init__.py file is required.

If you agree, I'll PR adding back the empty __init__.py file.

Yes, ordinary packages shadow namespace packages. A configuration in which an old version of Sage is installed in system site-packages cannot be reliably supported.

I don't see why. I've been running development sage-the-library for a while without issues. Note that the problem here arises only because the empty __init__.py exists in the system site-packages but not in our local PYTHONPATH. Following the rule I quoted above will prevent this issue from happening.

In other words, we have to make sure that everything that the sage library may want to import actually exists in our local installation. I.e. every module but also the whole path of packages containing it.

[mkoeppe]

Since sage.rings.number_field is a package (it contains modules) the __init__.py file is required.

No, this is no longer true since PEP 420. Read https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#ordinary-packages-vs-implicit-namespace-packages

[tornaria]

Since sage.rings.number_field is a package (it contains modules) the __init__.py file is required.

No, this is no longer true since PEP 420. Read https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#ordinary-packages-vs-implicit-namespace-packages

I fail to see why you needed to remove

src/sage/rings/number_field/__init__.py
src/sage/rings/polynomial/__init__.py
src/sage/rings/finite_rings/__init__.py

You are not moving these modules to a separate package, are you? They are still part of sagemath-standard AFAICT.

If kind of silly to modularize sage-the-library breaking it in smaller more manageable parts, if making system packages for those parts is not supported. The whole point of installing sage-the-library in the system site-packages is so that it can be imported from regular python. At least I thought that was the plan...

If you really need to remove those __init__.py files please document what is broken when they are present and/or give me a way out of this mess.

---

These are the directories that may be missing __init__.py:

$ for d in $(find src/sage -type d) ; do ls $d/__init__* &> /dev/null || ((ls $d/*.py* &> /dev/null || ls $d/*.px* &> /dev/null) && echo $d ) ; done
src/sage
src/sage/matrix
src/sage/ext
src/sage/rings
src/sage/rings/number_field
src/sage/rings/polynomial
src/sage/rings/finite_rings
src/sage/numerical
src/sage/numerical/backends
src/sage/graphs
src/sage/graphs/graph_decompositions
src/sage/sets
src/sage/categories
src/sage/interfaces
src/sage/misc
src/sage/libs
src/sage/arith
src/sage/ext_data/nbconvert

The last one is a false positive, but all the others seem reasonable to me. I don't quite care if the packages have or not __init__.py files. What causes the problem is changing a package from having to not-having __init__.py.

[mkoeppe]

I fail to see why you needed to remove

src/sage/rings/number_field/__init__.py
src/sage/rings/polynomial/__init__.py
src/sage/rings/finite_rings/__init__.py

You are not moving these modules to a separate package, are you?

Yes, I am moving some of the contained modules to separate distributions.

Technology preview: #35095 – this enlarges the distribution sagemath-categories to include a bit from each of these 3 namespace packages. See https://github.com/sagemath/sage/blob/2b264b236844b196e360de300701fbe5fc977636/pkgs/sagemath-categories/MANIFEST.in.m4
It includes the base classes of number fields, but no number field implementations other than the rationals.
It includes polynomials, enough for basic operations with univariate and multivariate polynomials, but no specialized element implementations that pull in dependencies on various libraries.
Likewise for finite rings.

[mkoeppe]

They are still part of sagemath-standard AFAICT.

Yes, at the moment sagemath-standard is still monolithic. But #34587 makes sagemath-standard depend on subset distributions such as sagemath-categories.

[tornaria]

You are not moving these modules to a separate package, are you?

Yes, I am moving some of the contained modules to separate distributions.

Is this happening for 10.0? If not, can we just keep these at least until 10.0 is released? Better yet, can you remove them as part of the actual move to separate distributions? I assume that will be a lot of pain for distributions, we may as well get all the pain at the same time. Maybe I'll get my sagemath package stuck at 10.0 forever.

Another thought: why is it so important to remove __init__.py ? Can't we just keep it in sagemath-standard? The same file cannot belong to two separate distributions, but it could belong to the main one, couldn't it?

I still fail to see what problem this is solving.

[mkoeppe]

why is it so important to remove __init__.py ? Can't we just keep it in sagemath-standard? The same file cannot belong to two separate distributions, but it could belong to the main one, couldn't it?

No. If any distribution ships an __init__.py file, then it makes the portions of that package provided by other distributions invisible.

[mkoeppe]

I don't know what's special about 10.0

[mkoeppe]

I still fail to see what problem this is solving.

The modularization solves the problem that nobody can install and use parts of Sage without pulling in gigabytes of non-pip-installable dependencies.

[mkoeppe]

I do agree that some synchronization may help – because the issue that you have reported arises from the transition to namespace packages; it is not a problem by itself.
So I will prepare a PR that removes all the __init__.py files of all packages that are intended to become namespace packages.

[tornaria]

I don't know what's special about 10.0

Nothing special, I'm just saying don't break releases if the PR that need this are not going to be merged.

I still fail to see what problem this is solving.
The modularization solves the problem that nobody can install and use parts of Sage without pulling in gigabytes of non-pip-installable dependencies.

That's what distros are for. The sagemath package in void linux is just 50M compressed (builds in 10 minutes from 28M source). What is the advantage of shipping it in separate chunks, if they are developed together and they can't be updated separately?

I do agree that some synchronization may help – because the issue that you have reported arises from the transition to namespace packages; it is not a problem by itself. So I will prepare a PR that removes all the __init__.py files of all packages that are intended to become namespace packages.

Now, that's constructive. Let's do it explicitly and knowing we break stuff. What about this:
a. we restore in the next beta the 3 empty __init__.py files that were removed in beta5, so we don't break development in the presence of a system install of 9.8
b. we coordinate with distros so that the system install of sage 10.0 will not include a given list of empty __init__.py files
c. after 10.0 is available on distros without these empty files, we remove them in a future release cycle.

Maybe if there was an easy and well documented way to install sage-the-library system wide (e.g. for a distro) we could impose some policy to distro-packages (like not installing the empty __init__.py files in site-packages).

---

Let me ask you a question in the opposite direction: do you know why are all those empty __init__.py files needed for? IOW, what would happen if I just remove all the empty __init__.py files from site-packages? As I understand PEP 420, there is no reason to keep them if python 2 support is not needed. Is there a measurable effect of removing all them?

[mkoeppe]

The modularization solves the problem that nobody can install and use parts of Sage without pulling in gigabytes of non-pip-installable dependencies.

That's what distros are for.

There are lots of people out there who don't use distro Python.

Basically, if something is not pip-installable, it's not usable as an upstream for other Python projects.

[mkoeppe]

By the way, does Void have any mechanism for users to make Python venvs that mix a defined set of Void-packaged Python distribution packages with pip-installed Python packages?

[mkoeppe]

Let me ask you a question in the opposite direction: do you know why are all those empty __init__.py files needed for? IOW, what would happen if I just remove all the empty __init__.py files from site-packages? As I understand PEP 420, there is no reason to keep them if python 2 support is not needed. Is there a measurable effect of removing all them?

There is no technical need for any of the empty __init__.py files. You can remove them.
Basically, they are in our source tree because they have not been removed. However:

• In our build system, when there is no __init__.py file, there needs to be an all*.py file to ensure that the modules inside it are found. (At runtime, they are not needed – except for running the doctester.)
• It may be valuable to keep some __init__.py files as documentation that this package is not intended to be made a namespace packages, as in "it's not welcome for other distributions to add stuff here".