tox -e relint: Do not complain about imports from sage.categories.all

[mkoeppe]

A refinement of #32899, #32879:

sage.categories is a namespace package: The most basic categories needed for coercion are distributed in sagemath-objects; everything else in sagemath-categories.

However, we now declare imports from sage.categories.all to be OK.

Rationale: Categories are supposed to be abstract - so there should not be compile-time or module-load-time dependencies.

CC: @kwankyu @fchapoton @tscrim @tobiasdiez

Component: categories

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/33202

[mkoeppe]

Branch: u/mkoeppe/tox__e_relint__do_not_complain_about_imports_from_sage_categories_all

[mkoeppe]

New commits:

17dc05c	src/.relint.yml: Allow imports from sage.categories.all
f18fa73	src/doc/en/developer/packaging_sage_library.rst: Allow imports from sage.categories.all

[mkoeppe]

Description changed:

--- 
+++ 
@@ -1,6 +1,8 @@
+A refinement of #32899, #32879:
+
 `sage.categories` is a namespace package: The most basic categories needed for coercion are distributed in **sagemath-objects**; everything else in **sagemath-categories**.
 
-We declare imports from `sage.categories.all` to be OK.
+However, we now declare imports from `sage.categories.all` to be OK.
 
-Categories are supposed to be abstract - so there should not be compile-time or module-load-time dependencies.
+Rationale: Categories are supposed to be abstract - so there should not be compile-time or module-load-time dependencies. 
 

[mkoeppe]

Commit: f18fa73

[mkoeppe]

Author: Matthias Koeppe

[tobiasdiez]

comment:4

I don't quite understand your "rationale". Can you please expand on what these all imports have to do with dependencies?

One of the problems with all imports is that this loads all modules referenced from the all module, potentially leading to cyclic imports and decreased performance. This also applies to the categories package, so I don't see why this should be an exception.

[mkoeppe]

comment:5

Replying to @tobiasdiez:

One of the problems with all imports is that this loads all modules referenced from the all module, potentially leading to cyclic imports and decreased performance. This also applies to the categories package, so I don't see why this should be an exception.

All of this is true both for namespace packages and ordinary packages.

The policy to avoid .all imports is specifically for namespace packages and is motivated not by these issues, but rather by the modularization effort. The documentation added in #32899 explains this - https://github.com/sagemath/sage-prod/blob/develop/src/doc/en/developer/packaging_sage_library.rst#module-level-runtime-dependencies

[tobiasdiez]

comment:6

Thanks for the explanation but I still don't get why there is a distinction between namespace packages and ordinary packages when it comes to the all import.
The docs you referred to also only mention

The main purpose of the :mod:*.all modules is for populating the global interactive environment that is available to users at the sage: prompt.

as the reason why all should be avoided, but this should also apply to ordinary packages, and to the categories in particular.

But since we seem to agree that all imports should be avoided in general, what about adding a second relint rule matching all such imports (for all sage modules) with error: false to mark it as a warning for now. Maybe also then reduce the existing rule to only those imports that are already no longer used (or for which there exists tickets), which has the positive side-effect that the linter is green again. This would be similar to the pycodestyle rules, for which we have a minimal and maximal config.

[mkoeppe]

comment:7

Replying to @tobiasdiez:

I still don't get why there is a distinction between namespace packages and ordinary packages when it comes to the all import.
The docs you referred to also only mention

The main purpose of the :mod:*.all modules is for populating the global interactive environment that is available to users at the sage: prompt.

The whole section leading to this little paragraph explains why managing dependencies is necessary for the modularization.

For a namespace package, if not all distributions that fill it are installed, then the .all module cannot be successfully imported.

An ordinary package is provided by a single distribution; so if the .all module is present, it can also be imported. Yes, importing it may import more than what is needed. But it works and this is the status quo in Sage.

The purpose of the linting rule is to enforce what is necessary for the modularization. It is mildly inconvenient for developers, but because it is necessary for the modularization to work, it is sufficiently motivated.

since we seem to agree that all imports should be avoided in general

No, for example there's little point in replacing from sage.plot.all import .... by more specific imports. When you starting plotting stuff, it's likely that you need most of the modules in this package.

[tobiasdiez]

comment:8

Okay, thanks I think I start to understand now.

But how is categories.all now different? If I understand the ticket description correctly, you are saying that not everything in categories is distributed in sagemath-objects. So if another module imports things in sagemath-objects through categories.all, then it needs sagemath-categories as dependency.

No, for example there's little point in replacing from sage.plot.all import .... by more specific imports.

Except for that it would align with general Python conventions, would be more coherent (why should one care about ordinary packages vs namespace packages while importing?) and future-proof (who knows what will be added to plot in the future...). Also plot.all contains a few things like animated plots or special plot for matrices that one probably doesn't need if all one wants to do is plot a line.

[mkoeppe]

comment:11

No longer needed

[mkoeppe]

Changed author from Matthias Koeppe to none

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

Changed commit from f18fa73 to none

[tscrim]

Changed branch from u/mkoeppe/tox__e_relint__do_not_complain_about_imports_from_sage_categories_all to none