Developer Guide: Document practices for data files #37399

mkoeppe · 2024-02-19T18:40:45Z

Topics here:

ext_data is legacy location only, see Meta-ticket: Remove use of SAGE_LIB and SAGE_EXTCODE variables #33037
separate repos for large data files
importlib_resources

Follow-ups:

use of Features

📝 Checklist

The title is concise, informative, and self-explanatory.
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 accordingly.

⌛ Dependencies

mkoeppe · 2024-02-19T18:42:01Z

@tornaria @gmou3 Let's collaborate here to bring our documentation up to date on these topics.

tornaria · 2024-02-19T19:26:00Z

src/doc/en/developer/coding_basics.rst

+- Large data files should not be added to the Sage source tree.
+  Instead:
+
+  - Create a separate git repository for them
+  - Add metadata in your repository that make it a pip-installable
+    package (distribution package)
+  - Upload it to PyPI
+  - Create metadata in ``SAGE_ROOT/build/pkgs`` that make your new
+    pip-installable package known to Sage
+


Nice. Some ideas:

give https://github.com/sagemath/conway-polynomials and https://github.com/gmou3/matroid-database as examples

suggest compressing large data files with xz, read transparently as in https://github.com/sagemath/conway-polynomials/blob/master/src/conway_polynomials/__init__.py#L116

suggest trying xz -T 0 -e to compress, sometimes gives big gains for highly repetitive text files (without affecting the cost for decompression).

Thanks! Yes, linking to these projects as examples of best practices is a good idea. Will do (unless someone sends me a PR first... hint, hint)

@tornaria are these points taken care of by the current version of the text already?

tornaria · 2024-02-19T19:48:37Z

For accessing resources in packages, link to https://docs.python.org/3/library/importlib.resources.html#importlib.resources.files. Note everything else is deprecated.

Basic examples:

open a resource for text reading: fd = files(package).joinpath(resource).open('rt')
open a resource for binary reading: fd = files(package).joinpath(resource).open('rb')
read a resource as text: text = files(package).joinpath(resource).read_text()
read a resource as bytes: bytes = files(package).joinpath(resource).read_bytes()
open a xz resource for text reading: fd = lzma.open(files(package).joinpath(resource), 'rt')
open a xz resource for binary reading: fd = lzma.open(files(package).joinpath(resource), 'rb')

Note: as_files() is only necessary if one needs an actual file by path, e.g. to pass to an external command. AFAICT, it's not needed to use as in the examples above.

Edit: I added explicit 'rt' to text mode since open() and lzma.open() have different meanings for 'r'.

tornaria · 2024-02-19T20:02:38Z

Notes:

I think in both conway-polynomials and matroid-database, the construct TextIOWrapper(lzma.open(FILE)) is equivalent to lzma.open(FILE, 'r')
Is the as_files(dbpath) really necessary in https://github.com/sagemath/conway-polynomials/blob/master/src/conway_polynomials/__init__.py#L117 ?
Is the .open("rb") really necessary in https://github.com/gmou3/matroid-database/blob/main/src/matroid_database/__init__.py#L89 ?

gmou3 · 2024-02-19T20:20:39Z

This code looks nice to me:

import lzma
from importlib.resources import files

path = files(module).joinpath(name).with_suffix(".txt.xz")
try:
    return lzma.open(path, 'rt')
except FileNotFoundError:

It seems like we want the 'rt' option: https://docs.python.org/3/library/lzma.html.

tornaria · 2024-02-19T20:54:47Z

It seems like we want the 'rt' option: https://docs.python.org/3/library/lzma.html.

You are right; a different default from io.open() and also a different meaning of 'r' 🤷

tornaria · 2024-02-20T01:51:34Z

src/doc/en/developer/coding_basics.rst

+     This is no longer recommended, and PRs that update such uses
+     are welcome.


We should not un-recommend this until sage.interfaces is a package or we remove support for python 3.9. The alternative is ugly, and is adding code that will have to be replaced again soon.

adding code that will have to be replaced again soon

why is this?

importlib_resources is only needed for namespace packages for python 3.9 so your try hack is unecessary with python 3.10.

Please. It's not a "hack". These are the best practices.

And by documenting this method to import it, we avoid the "too much information" problem where we tell developers every little detail of what is supported on what version etc.

Please. It's not a "hack". These are the best practices.

According to whom?

OK, maybe second-best practices

Now (with abf3ae6), at best practices.

Please give an external reference (i.e. not yourself)

Hard to tell what you are looking for. https://importlib-resources.readthedocs.io/en/latest/using.html# leaves no doubt how to import from it. We have decided to conditionalize the dependency on the backport package in #36776 @orlitzky, hence here we conditionalize the import.

In case you are looking for this: You can see how it is used in projects in the wild in this code search:
https://github.com/search?q=%22from+importlib_resources+import%22&type=code&p=1

We should not un-recommend this until ...

@tornaria I've changed it in 4d38922, please take a look.

mkoeppe · 2024-02-20T01:57:37Z

@orlitzky @soehms You might be interested in this PR as well

orlitzky · 2024-02-20T12:08:43Z

Notes:

1. I think in both conway-polynomials and matroid-database, the construct `TextIOWrapper(lzma.open(FILE))` is equivalent to `lzma.open(FILE, 'r')`

2. Is the `as_files(dbpath)` really necessary in https://github.com/sagemath/conway-polynomials/blob/master/src/conway_polynomials/__init__.py#L117 ?

They make the type-checker (mypy) happy.

tornaria · 2024-02-20T16:38:43Z

src/doc/en/developer/coding_basics.rst

+    try:
+        # Use backport package providing Python 3.11 features
+        from importlib_resources import files
+    except ImportError:
+        from importlib.resources import files


Suggested change

try:

# Use backport package providing Python 3.11 features

from importlib_resources import files

except ImportError:

from importlib.resources import files

from importlib.resources import files

maybe with a note about not using resources in namespace packages until 3.9 is removed support, i.e. apr 2024?. Really not worth doing this for 2 months and using the version of importlib.resources from python is better to avoid bleeding edge and stick to long term stable API.

Your code allows one, in principle, to use features from new importlib_resources which are not available in the version of python used. But if those features are indeed used, this results in an incompatibility with a system where python is new enough to have importlib.resources but where importlib_resources is not installed (or not up to date). From that point of view is much better to stick to the stable API, meaning features that are common available in all the supported versions of python.

The only newer nicety I see (available on 3.12) is that one can use files() to stand for files(__package__)

The case of "importlib_resources not up to date" is a real point. I'll make a change.

I don't agree with the rest what you're saying.

In abf3ae6, I've changed it to using an explicit version check (for 3.11!) instead of try... except.

I'm using 3.11, not 3.9.
Note the section on "Language Standard" in the Developer's Guide: The idea is that we develop the Sage library to one language standard, and that's currently 3.9 + the backports for 3.11.

until 3.9 is removed support, i.e. apr 2024?

We don't have a set schedule for this yet.

The only newer nicety I see (available on 3.12) is that one can use files() to stand for files(package)

That's quite nice, so let's please merge

exceptiongroup, importlib_metadata, importlib_resources, typing_extensions: Require 3.12 features #37402

until 3.9 is removed support, i.e. apr 2024?

We don't have a set schedule for this yet.

42 months as per NEP 29 is apr 2024. They call this best practices.

Please @tornaria. Our project has not adopted NEP 29. If you want to know about that, read
https://github.com/sagemath/sage/wiki/NEP-29:-Python-version-strategy

src/doc/en/developer/coding_basics.rst

tornaria · 2024-02-20T17:54:53Z

src/doc/en/developer/coding_basics.rst

+    if sys.version_info < (3, 11):
+        # Use backport package providing Python 3.11 features
+        from importlib_resources import files
+    else:
+        from importlib.resources import files


Suggested change

if sys.version_info < (3, 11):

# Use backport package providing Python 3.11 features

from importlib_resources import files

else:

from importlib.resources import files

from importlib.resources import files

I already gave rationale.

You already know that I disagree, and I have explained it.

If you don't agree with the suggestion, feel free to label the PR as disputed.

That's not how it works.

I have taken this part out now to regain velocity

mkoeppe · 2024-03-05T21:32:56Z

Let's get this in please.

orlitzky · 2024-03-06T00:24:49Z

I agree with @tornaria that it would be better to use importlib.resources unconditionally, and simply add an __init__.py to the database packages in the meantime (I already did this with conway-polynomials). We would be better off without the three backport packages.

mkoeppe · 2024-03-06T00:27:05Z

But this document does not describe the use of importlib.resources in the standalone packages.
It describes its use in the Sage library.

orlitzky · 2024-03-06T01:09:03Z

But this document does not describe the use of importlib.resources in the standalone packages. It describes its use in the Sage library.

Ok, but same answer for all the same reasons.

mkoeppe · 2024-03-06T01:27:43Z

Well, no. The modularization of the Sage library and its uses of namespace packages does not disappear just like that.

orlitzky · 2024-03-06T01:52:50Z

The database should only be present in one distribution right? And if not, I think there's a decent consensus that we should chill for a while on the init file removal until namespace packages are well-supported upstream.

mkoeppe · 2024-03-06T03:05:19Z

I think there's a decent consensus that we should chill for a while on the init file removal until namespace packages are well-supported upstream.

No, Michael, there is no such consensus, and certainly not a "decent" one. That the Sage library uses namespace packages for the purpose of the modularization is documented policy in the developer's guide.

mkoeppe · 2024-03-06T03:08:25Z

The database should only be present in one distribution right?

For the separate distributions (conway-polynomials, matroid-database), using an ordinary package (with __init__.py) is of course the right choice, as they control their own namespaces.

mkoeppe · 2024-03-06T03:19:26Z

What we are trying to document here is to cover cases such as SAGE_ROOT/src/sage/interfaces/maxima.lisp.
sage.interfaces is a namespace package, and in fact one that is designed to be split over 6 distribution packages:
https://github.com/sagemath/sage/pull/36676/files#diff-2af172ee945eebb48bbe8a3cdd6b523221c1a995b2e4f8ac23fa7c3e1181e6ef

orlitzky · 2024-03-06T03:33:47Z

No, Michael, there is no such consensus, and certainly not a "decent" one. That the Sage library uses namespace packages for the purpose of the modularization is documented policy in the developer's guide.

It's in the developer's guide because you put it there.

What we are trying to document here is to cover cases such as SAGE_ROOT/src/sage/interfaces/maxima.lisp.
sage.interfaces is a namespace package, and in fact one that is designed to be split over 6 distribution packages:

Handled by doing nothing and waiting for namespace package support upstream. Nobody needs this by tomorrow, and the maintenance to keep it working is a headache.

mkoeppe · 2024-03-06T03:39:24Z

None of this sounds like the beginning of an interesting discussion; rather it sounds like the end of a discussion.

tornaria · 2024-03-16T21:24:38Z

src/doc/en/developer/coding_in_python.rst

+    (to be used in place of ``importlib.metadata`` when Python older
+    than 3.11 is in use),
+  - `importlib_resources <../reference/spkg/importlib_resources>`_:
+    If necessary, you can use it in place of ``importlib.resources``
+    when Python older than 3.11 is in use if you need a newer
+    feature::
+
+        import sys
+
+        if sys.version_info < (3, 11):
+            # Use backport package providing Python 3.11 features
+            from importlib_resources import files
+        else:
+            from importlib.resources import files
+
  - `typing_extensions <../reference/spkg/typing_extensions>`_
-    (to be used in place of ``typing``).
+    (to be used in place of ``typing`` when any of the features
+    introduced after Python 3.9.0 are used).


You forgot about this, other that this the PR looks good to me.

I would remove almost all of this section, starting from "Some key language and library functions..." to the end, but I understand if you don't want to do it here.

I didn't forget about it. I actually changed the language so it does not unconditionally asks for it to be imported like that.

~~Please, let's not play games.~~

Two reviewers showed concern for this. You can have a different opinion, but if you accept to take it out "to regain velocity", then do it ~~without tricks~~ fully. I already stated my positive review conditional on this.

Edit: toned it down. I didn't intend to offend @mkoeppe at all. The underlying message is a constructive review to get the 95% of your PR that we all agree is a very nice improvement.

Gonzalo, this is uncalled for.

What is your concern about this improvement of the documentation in the "Language Standard" section?

I'm not really familiar with the functionality discussed here. The only thing I can do is moderate the conflict (if my understanding of it is good enough). Could this be a compromise: we leave the example code-block as is and add a note that some distributions no longer ship importlib_resources for good reasons. Ideally we should add some URL pointers that address these good reasons.

I'm not really familiar with the functionality discussed here. The only thing I can do is moderate the conflict (if my understanding of it is good enough). Could this be a compromise: we leave the example code-block as is and add a note that some distributions no longer ship importlib_resources for good reasons. Ideally we should add some URL pointers that address these good reasons.

Sorry I missed this before.

It seems we don't agree whether to recommend or not using importlib_resources. In this
context, I think merging this nice PR without this small snippet is a good way to go, and I thought that's what @mkoeppe was proposing. That's why I gave it positive review conditional on this.

I'll split out the improvement of this section of the manual (orthogonal to the goal of this PR) out to a separate PR and will take @soehms's suggestion into account there.

Done now in the rebased branch.

... and in #37654.

soehms · 2024-03-18T18:51:14Z

src/doc/en/developer/coding_basics.rst

+  location as the Python code. This is referred to as "package data".
+
+  The preferred way to access the data from Python is using the
+  `importlib.resources API <https://importlib-resources.readthedocs.io/en/latest/using.html>`,


This line doesn't look rendered correctly when I follow the documentation link.

Fixed in 6045e5c, I hope

mkoeppe · 2024-03-23T00:46:00Z

Other follow-ups:

…e added to the Sage source tree

…ctions

…iles

…resources.files

…s_file, add links to tutorial

…of try..except

…amples Co-authored-by: Gonzalo Tornaría <[email protected]>

…te direct access to data files using __file__ for now

…GE_EXTCODE deprecation

…t from importlib.resources, fix markup

mkoeppe · 2024-03-26T04:37:59Z

Let's get this in please.

github-actions · 2024-03-27T07:52:16Z

Documentation preview for this PR (built with commit d3a4313; changes) is ready! 🎉

mkoeppe · 2024-03-28T03:55:14Z

Thanks.

…kages       - sagemath#35203 added these packages as unconditional dependencies - prompted by a sporadic use of typing_extensions in the Sage library (see sagemath#34831) - motivated further by the demand to immediately drop support for Python 3.8 so that newer typing features can be used in the Sage library (sagemath#35404) - sagemath#36776 reduced the packages to conditional dependencies Here we improve the documentation in the section "Language standard" of the developer's guide so that it aligns with how the conditional dependencies are declared. Based on changes split out from sagemath#37399. ### 📝 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 accordingly. ### ⌛ Dependencies    URL: sagemath#37654 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Matthias Köppe

tornaria reviewed Feb 19, 2024

View reviewed changes

mkoeppe added the c: documentation label Feb 20, 2024

tornaria reviewed Feb 20, 2024

View reviewed changes

src/doc/en/developer/coding_basics.rst Outdated Show resolved Hide resolved

tornaria reviewed Feb 20, 2024

View reviewed changes

tornaria mentioned this pull request Feb 20, 2024

Readd init files #36753

Closed

5 tasks

mkoeppe force-pushed the document_package_data_practices branch 2 times, most recently from 1bbc922 to 8dcb34a Compare February 26, 2024 23:16

mkoeppe marked this pull request as ready for review February 26, 2024 23:18

mkoeppe added the s: needs review label Feb 26, 2024

mkoeppe force-pushed the document_package_data_practices branch from 8dcb34a to 6984347 Compare March 5, 2024 21:32

mkoeppe requested a review from soehms March 16, 2024 19:18

tornaria reviewed Mar 16, 2024

View reviewed changes

soehms reviewed Mar 18, 2024

View reviewed changes

mkoeppe force-pushed the document_package_data_practices branch from f9955ca to 3cfab54 Compare March 22, 2024 21:12

mkoeppe requested a review from kiwifb March 24, 2024 21:14

Matthias Koeppe and others added 13 commits March 25, 2024 21:37

src/doc/en/developer/coding_basics.rst: Large data files should not b…

18d2eb7

…e added to the Sage source tree

Add compression suggestion and guiding example repos (#26)

894b667

src/doc/en/developer/coding_basics.rst: Add links to packaging instru…

e9038c5

…ctions

src/doc/en/developer/coding_basics.rst: Mention importlib.resources.f…

0f74867

…iles

src/doc/en/developer/coding_basics.rst: Show how to import importlib.…

59a5204

…resources.files

src/doc/en/developer/coding_basics.rst: Mention importlib.resources.a…

827372e

…s_file, add links to tutorial

src/doc/en/developer/coding_basics.rst: Add importlib.resources examples

a72e2fa

src/doc/en/developer/coding_basics.rst: Use sys.version_info instead …

bab3d03

…of try..except

src/doc/en/developer/coding_basics.rst: Details on resource open ex…

515b4b3

…amples Co-authored-by: Gonzalo Tornaría <[email protected]>

src/doc/en/developer/coding_basics.rst: Don't encourage PRs that upda…

421c37f

…te direct access to data files using __file__ for now

src/doc/en/developer/coding_basics.rst: Link to sagemath#33037 for SA…

0c28f6f

…GE_EXTCODE deprecation

src/doc/en/developer/coding_basics.rst: Remove advice on how to impor…

81741c5

…t from importlib.resources, fix markup

src/doc/en/developer/coding_basics.rst: Fix markup, break long lines

d3a4313

mkoeppe force-pushed the document_package_data_practices branch from 6045e5c to d3a4313 Compare March 26, 2024 04:37

tornaria added s: positive review and removed s: needs review labels Mar 26, 2024

vbraun merged commit e6a377e into sagemath:develop Mar 31, 2024
14 of 15 checks passed

mkoeppe added this to the sage-10.4 milestone Mar 31, 2024

github-actions bot removed the s: positive review label Apr 1, 2024

mkoeppe deleted the document_package_data_practices branch May 5, 2024 00:09

		This is no longer recommended, and PRs that update such uses
		are welcome.

Developer Guide: Document practices for data files #37399

Developer Guide: Document practices for data files #37399

Conversation

mkoeppe commented Feb 19, 2024 • edited Loading

📝 Checklist

⌛ Dependencies

mkoeppe commented Feb 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkoeppe Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

tornaria commented Feb 19, 2024 • edited Loading

tornaria commented Feb 19, 2024

gmou3 commented Feb 19, 2024

tornaria commented Feb 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkoeppe commented Feb 20, 2024

orlitzky commented Feb 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkoeppe commented Mar 5, 2024

orlitzky commented Mar 6, 2024

mkoeppe commented Mar 6, 2024

orlitzky commented Mar 6, 2024

mkoeppe commented Mar 6, 2024

orlitzky commented Mar 6, 2024

mkoeppe commented Mar 6, 2024

mkoeppe commented Mar 6, 2024

mkoeppe commented Mar 6, 2024

orlitzky commented Mar 6, 2024

mkoeppe commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tornaria Mar 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkoeppe commented Mar 23, 2024

mkoeppe commented Mar 26, 2024

github-actions bot commented Mar 27, 2024

mkoeppe commented Mar 28, 2024

mkoeppe commented Feb 19, 2024 •

edited

Loading

mkoeppe Feb 20, 2024 •

edited

Loading

tornaria commented Feb 19, 2024 •

edited

Loading

tornaria Mar 16, 2024 •

edited

Loading