Opened 7 weeks ago

Closed 5 weeks ago

Last modified 3 weeks ago

#31921 closed defect (fixed)

Knotinfo db interface looks for the wrong python module

Reported by: fbissey Owned by:
Priority: blocker Milestone: sage-9.4
Component: packages: optional Keywords:
Cc: soehms, tscrim, arojas Merged in:
Authors: François Bissey Reviewers: Sebastian Oehms
Report Upstream: N/A Work issues:
Branch: 9907095 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by fbissey)

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.

Change History (23)

comment:1 Changed 7 weeks ago by tscrim

  • Cc soehms tscrim added

comment:2 Changed 7 weeks ago by fbissey

  • Authors set to François Bissey
  • Branch set to u/fbissey/knotinfo_feature
  • Commit set to 7fe6cf9eba3c30bc6157ce3de897eb2f6363cb26
  • Status changed from new to needs_review

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:

7fe6cf9Replace PythonModule feature by StaticFile feature.

comment:3 follow-ups: Changed 7 weeks ago by 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')

and set self._sobj_path to something like database_knotinfo.__file__

comment:4 in reply to: ↑ 3 Changed 7 weeks ago by fbissey

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.

comment:5 in reply to: ↑ 3 Changed 7 weeks ago by fbissey

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?

comment:6 Changed 7 weeks ago by mkoeppe

yes, it needs to be imported

comment:7 Changed 7 weeks ago by fbissey

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.

comment:8 follow-up: Changed 7 weeks ago by 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?)

comment:9 in reply to: ↑ 8 ; follow-up: Changed 7 weeks ago by fbissey

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.

comment:10 Changed 7 weeks ago by arojas

  • Cc arojas added

comment:11 in reply to: ↑ 9 Changed 7 weeks ago by soehms

Replying to fbissey:

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!

comment:12 Changed 7 weeks ago by git

  • Commit changed from 7fe6cf9eba3c30bc6157ce3de897eb2f6363cb26 to 0091a4e879a1c10e7eb0feb4f0408337ab7ad04f

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

0091a4eUse PythonModule, but this time the right one

comment:13 Changed 7 weeks ago by fbissey

  • Status changed from needs_review to needs_work

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.

comment:14 Changed 7 weeks ago by git

  • Commit changed from 0091a4e879a1c10e7eb0feb4f0408337ab7ad04f to 99070951e1888e5a0bafd19cd1d88215536b67e3

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

9907095Define _sobj_path in knotinfo_db.py itself

comment:15 Changed 7 weeks ago by fbissey

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

comment:16 Changed 7 weeks ago by fbissey

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:17 Changed 7 weeks ago by fbissey

  • Summary changed from The knotinfo db should use the StaticFile feature, not PythonModule to Knotinfo db interface looks for the wrong python module

comment:18 Changed 7 weeks ago by soehms

  • Reviewers set to Sebastian Oehms
  • Status changed from needs_review to positive_review

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!

comment:19 Changed 7 weeks ago by mkoeppe

I also see this error after installing pytest.

comment:20 follow-up: Changed 7 weeks ago by mkoeppe

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

comment:21 in reply to: ↑ 20 Changed 7 weeks ago by soehms

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!

comment:22 Changed 5 weeks ago by vbraun

  • Branch changed from u/fbissey/knotinfo_feature to 99070951e1888e5a0bafd19cd1d88215536b67e3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:23 Changed 3 weeks ago by soehms

  • Commit 99070951e1888e5a0bafd19cd1d88215536b67e3 deleted

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

Note: See TracTickets for help on using tickets.