Knotinfo db interface looks for the wrong python module

kiwifb commented 3 years ago

Ticket #30352 included a new optional database and its interface in sage. As all new optional package of this kind, whether to use it or not is detected at runtime by the feature framework.

In #30352 presence was implemented by looking if the sage interface python module is installed. Which is always true since this interface is shipped in the standard sagelib. Instead it should have searched for the module installed by the database_knotinfo package itself.

Furthermore, it sets user writable cache under SAGE_SHARE which is a system location for any system wide installation of sage.

The problem is not clearly visible in vanilla sage as there are no direct consequence for a private user installation. But for system installation or distro installation which are sandboxed, there are consequences as sage will try to create files in system location. See https://github.com/cschwan/sage-on-gentoo/issues/639 for example.

CC: @soehms @tscrim @antonio-rojas

Component: packages: optional

Author: François Bissey

Branch: 9907095

Reviewer: Sebastian Oehms

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

kiwifb commented 3 years ago

comment:2

Attach branch with patch currently tested in sage-on-gentoo. That allows documentation to be built, I am currently running doctests. The next test would be to make sure it behaves properly with the knotinfo package installed.

New commits:

7fe6cf9 Replace PythonModule feature by StaticFile feature.

kiwifb commented 3 years ago

Branch: u/fbissey/knotinfo_feature

kiwifb commented 3 years ago

Author: François Bissey

kiwifb commented 3 years ago

Commit: 7fe6cf9

mkoeppe commented 3 years ago

comment:3

This is not the right fix. The database is provided by a Python package so it should be looked up via import machinery.

Just change:

- PythonModule.__init__(self, 'sage.knots.knotinfo', spkg='database_knotinfo',)
+ PythonModule.__init__(self, 'database_knotinfo', spkg='database_knotinfo')

and set self._sobj_path to something like database_knotinfo.__file__

kiwifb commented 3 years ago

comment:4

Replying to @mkoeppe:

This is not the right fix. The database is provided by a Python package so it should be looked up via import machinery.

Just change:
- PythonModule.__init__(self, 'sage.knots.knotinfo', spkg='database_knotinfo',)
+ PythonModule.__init__(self, 'database_knotinfo', spkg='database_knotinfo')

I understand, the wrong python module was looked up. Of course you cannot know that without installing the package or reading the ticket very, very, carefully.

I will put up a corrected branch later today.

kiwifb commented 3 years ago

comment:5

Replying to @mkoeppe:

and set self._sobj_path to something like database_knotinfo.__file__

Python detail, do you need to import database_knotinfo before being able to query that path?

mkoeppe commented 3 years ago

comment:6

yes, it needs to be imported

kiwifb commented 3 years ago

comment:7

Hum, I have to package this to make sure I understand, but it looks to me that the current location of _sobj_path and what you suggest to do with .__file__ will be a quite different location. One should go under site-packages while the other will be under share.

mkoeppe commented 3 years ago

comment:8

OK, the _sobj_path is actually something else. It is a location of a cache that is created from the csv files distributed in the Python package. It's probably best to move this to a different location that is writable by users (somewhere in ~/.sage?)

kiwifb commented 3 years ago

comment:9

Replying to @mkoeppe:

OK, the _sobj_path is actually something else. It is a location of a cache that is created from the csv files distributed in the Python package. It's probably best to move this to a different location that is writable by users (somewhere in ~/.sage?)

Under ~/.sage should be the default for that kind of things yes. Should we put it in its own subfolder? I don't think it is necessary, as there seems to be only one object.

soehms commented 3 years ago

comment:11

Replying to @kiwifb:

Replying to @mkoeppe:

OK, the _sobj_path is actually something else. It is a location of a cache that is created from the csv files distributed in the Python package. It's probably best to move this to a different location that is writable by users (somewhere in ~/.sage?)

Under ~/.sage should be the default for that kind of things yes. Should we put it in its own subfolder? I don't think it is necessary, as there seems to be only one object.

There are more files, so that should be a separate folder.

Sorry for causing this trouble. I already had SAGE_DB (.sage/db) in mind instead of SAGE_SHARE when I did the last change in #30352. But since everything worked fine for me I let it as is.

I can do the tests but maybe not today!

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 7fe6cf9 to 0091a4e

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

0091a4e Use PythonModule, but this time the right one

kiwifb commented 3 years ago

comment:13

I am now understanding my confusion in full. All the other databases test for StaticFile and there was a file defined there in a "system location".

I'll add that since it is a user location, it doesn't really belongs to feature. Ideally, it would be defined in the package database_knotinfo itself. Let's find a location in sage for it for now, but we should keep it in mind next time the package is upgraded.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

9907095 Define _sobj_path in knotinfo_db.py itself

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 0091a4e to 9907095

kiwifb commented 3 years ago

comment:15

Need to rewrite the description but it should be a testable state now.

kiwifb commented 3 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,8 @@
 Ticket #30352 included a new optional database and its interface in sage. As all new optional package of this kind, whether to use it or not is detected at runtime by the feature framework.

-In #30352 presence was implemented by looking if the sage interface python module is installed. Which is always true since this interface is shipped in the standard sagelib. Instead it should have detected whether the database file was present.
+In #30352 presence was implemented by looking if the sage interface python module is installed. Which is always true since this interface is shipped in the standard sagelib. Instead it should have searched for the module installed by the `database_knotinfo` package itself.

-The problem is not clearly visible in vanilla sage as there are no direct consequence for a private user installation. But for system installation or distro installation which are sandboxed there are consequences as sage will try to create files in system location.
+Furthermore, it sets user writable cache under `SAGE_SHARE` which is a system location for any system wide installation of sage. 
+
+The problem is not clearly visible in vanilla sage as there are no direct consequence for a private user installation. But for system installation or distro installation which are sandboxed, there are consequences as sage will try to create files in system location.
 See https://github.com/cschwan/sage-on-gentoo/issues/639 for example.

soehms commented 3 years ago

comment:18

The patch looks good to me!

Another question: It seems that we have a new feature within our doctests, right now, which produces the following failure (independent on this branch):

sage -t --random-seed=0 src/sage/databases/knotinfo_db.py
    [90 tests, 0.06 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 0.2 seconds
    cpu time: 0.0 seconds
    cumulative wall time: 0.1 seconds
================================================================================================================================== test session starts ===================================================================================================================================
platform linux -- Python 3.9.2, pytest-6.2.4, py-1.10.0, pluggy-0.13.1
rootdir: /home/sebastian/devel/sage/src, configfile: tox.ini
collected 0 items / 1 error

========================================================================================================================================= ERRORS =========================================================================================================================================
_____________________________________________________________________________________________________________________ ERROR collecting sage/databases/knotinfo_db.py _____________________________________________________________________________________________________________________
src/sage/databases/knotinfo_db.py:330: in <module>
    class KnotInfoDataBase(SageObject, UniqueRepresentation):
sage/misc/nested_class.pyx:318: in sage.misc.nested_class.NestedClassMetaclass.__init__ (build/cythonized/sage/misc/nested_class.c:2327)
    ???
E   KeyError: 'knotinfo_db'
================================================================================================================================ short test summary info =================================================================================================================================
ERROR src/sage/databases/knotinfo_db.py - KeyError: 'knotinfo_db'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
==================================================================================================================================== 1 error in 0.73s ====================================================================================================================================

What is the reason for this? How can I fix it? Any hints are welcome!

soehms commented 3 years ago

Reviewer: Sebastian Oehms

mkoeppe commented 3 years ago

comment:19

I also see this error after installing pytest.

mkoeppe commented 3 years ago

comment:20

This defect is not specific to knotinfo_db; we will address it in #31924 (sage -t: Do not run pytest on individual files)

soehms commented 3 years ago

comment:21

Replying to @mkoeppe:

This defect is not specific to knotinfo_db; we will address it in #31924 (sage -t: Do not run pytest on individual files)

Many Thanks!

vbraun commented 3 years ago

Changed branch from u/fbissey/knotinfo_feature to 9907095

soehms commented 3 years ago

comment:23

There is another piece of code that should be adapted to the change of file-cache location. I've opened ticket #32099 for that!

soehms commented 3 years ago

Changed commit from 9907095 to none

sagemath / sage

Knotinfo db interface looks for the wrong python module #31921