#30349 closed defect (fixed)

Downgrade broken optional packages to experimental for Sage 9.2

Reported by: mkoeppe Owned by:
Priority: critical Milestone: sage-9.3
Component: packages: optional Keywords:
Cc: jhpalmieri, dimpase, slelievre, SimonKing Merged in:
Authors: Matthias Koeppe Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 674523c (Commits, GitHub, GitLab) Commit: 674523cf0bc34368e2aa5effb280c0dcf71f9808
Dependencies: #28711 Stopgaps:

Status badges

Description (last modified by mkoeppe)

see:

  • #29900 Meta-ticket: Fix optional and experimental packages for Sage 9.2

Downgrading two optional packages that fail to build on many platforms, as can be seen in https://github.com/mkoeppe/sage/actions/runs/296478189

Change History (35)

comment:1 Changed 22 months ago by mkoeppe

  • Cc slelievre added
  • Priority changed from critical to blocker

comment:2 Changed 22 months ago by vbraun

  • Priority changed from blocker to major

I'd say a broken optional package is by definition not a blocker...

comment:3 Changed 22 months ago by mkoeppe

  • Priority changed from major to critical

comment:4 Changed 21 months ago by mkoeppe

  • Branch set to u/mkoeppe/downgrade_broken_optional_packages_to_experimental_for_sage_9_2

comment:5 Changed 21 months ago by mkoeppe

  • Commit set to 402d45f5344ce17321164994d6e5eef7398f7819
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

402d45fbuild/pkgs/{deformation,p_group_cohomology}/type: Set to experimental

comment:6 Changed 21 months ago by mkoeppe

  • Authors set to Matthias Koeppe

comment:7 follow-up: Changed 21 months ago by dimpase

  • Cc SimonKing added

Simon, what is the current status of your package?

comment:8 in reply to: ↑ 7 ; follow-up: Changed 21 months ago by SimonKing

Replying to dimpase:

Simon, what is the current status of your package?

I thought that it works. As far as I recall, when I last checked, it did.

Anyway, I plan to give a talk on the package during the next SageDays?, so, I am motivated to fix it. Can you tell me what problems arose?

comment:9 in reply to: ↑ 8 Changed 21 months ago by SimonKing

Replying to SimonKing:

Replying to dimpase:

Simon, what is the current status of your package?

I thought that it works. As far as I recall, when I last checked, it did.

Anyway, I plan to give a talk on the package during the next SageDays?, so, I am motivated to fix it. Can you tell me what problems arose?

PS: By "my package", I refer to p_group_cohomology. There is "my other package", namely SharedMeatAxe, which is supposed to work and has recently also been fixed on cygwin (see #29152

comment:10 follow-ups: Changed 21 months ago by dimpase

  • Status changed from needs_review to needs_info

Here are results of a local run, with 9.2.beta14 - that's newer than this ticket, but a bit older than the current rc - they match errors seen in the logs in the ticket description. They appear to be a result of GAP files installed in a weird place, namely:

$SAGE_LOCAL/lib/python3.7/site-packages/$SAGE_LOCAL/share/sage/ext/gap/modular_cohomology/

rather than $SAGE_LOCAL/share/sage/ext/gap/modular_cohomology/. So I see

sage -t --random-seed=0 src/sage/tests/modular_group_cohomology.py
**********************************************************************
File "src/sage/tests/modular_group_cohomology.py", line 10, in sage.tests.modular_group_cohomology
Failed example:
    from pGroupCohomology import CohomologyRing   # optional - p_group_cohomology
Exception raised:
    Traceback (most recent call last):
      File "/home/scratch2/dimpase/sage/sage/local/lib64/python3.7/site-packages/sage/doctest/forker.py", line 720, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/scratch2/dimpase/sage/sage/local/lib64/python3.7/site-packages/sage/doctest/forker.py", line 1145, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.tests.modular_group_cohomology[0]>", line 1, in <module>
        from pGroupCohomology import CohomologyRing   # optional - p_group_cohomology
      File "/home/scratch2/dimpase/sage/sage/local/lib64/python3.7/site-packages/pGroupCohomology/__init__.py", line 2466, in <module>
        from pGroupCohomology.factory import CohomologyRing
      File "/home/scratch2/dimpase/sage/sage/local/lib64/python3.7/site-packages/pGroupCohomology/factory.py", line 43, in <module>
        from pGroupCohomology.auxiliaries import coho_options, coho_logger, safe_save, _gap_reset_random_seed, gap, singular, Failure
      File "/home/scratch2/dimpase/sage/sage/local/lib64/python3.7/site-packages/pGroupCohomology/auxiliaries.py", line 411, in <module>
        gap.Read(os.path.join(SAGE_SHARE,'sage','ext','gap','modular_cohomology','GapMaxels.g'))
      File "sage/libs/gap/element.pyx", line 2525, in sage.libs.gap.element.GapElement_Function.__call__ (build/cythonized/sage/libs/gap/element.c:19780)
        sig_on()
    sage.libs.gap.util.GAPError: Error, file "/home/scratch2/dimpase/sage/sage/local/share/sage/ext/gap/modular_cohomology/GapMaxels.g" must exist and be readable

(and more failing tests due to this, as you imagine) So the package looks for its GAP files in $SAGE_LOCAL/share/sage/ext/gap/modular_cohomology/

Perhaps Matthias knows the reason for this.

comment:11 follow-up: Changed 21 months ago by dimpase

It might be due to the packages now being built using Python wheels, and everything in the package goes to the site-packages/ of the Python, rather than to the local GAP.

comment:12 in reply to: ↑ 11 Changed 21 months ago by SimonKing

Replying to dimpase:

It might be due to the packages now being built using Python wheels, and everything in the package goes to the site-packages/ of the Python, rather than to the local GAP.

Probably.

So, tell me: is there an environment variable or a Sage variable that provides the location of GAP's package data?

Since this is an actual bug: Should it be fixed here (although this ticket is about downgrading several packages rather then fixing them) or on a new ticket?

comment:13 follow-up: Changed 21 months ago by SimonKing

Really unfortunate that the Cython code of p_group_cohomology has not been made optional Cython files in the Sage library, because then it would be easy to fix without upgrading the upstream source ball.

comment:14 in reply to: ↑ 10 ; follow-up: Changed 21 months ago by SimonKing

PS:

Replying to dimpase:

They appear to be a result of GAP files installed in a weird place, namely:

$SAGE_LOCAL/lib/python3.7/site-packages/$SAGE_LOCAL/share/sage/ext/gap/modular_cohomology/

It is indeed weird that $SAGE_LOCAL appears twice in this path. In the sources, if I see that correctly, this path appears only once, namely setup.py (data_files=[(os.path.join(SAGE_SHARE,"sage","ext","gap","modular_cohomology"))

So, if I understand correctly what the error says, GAP does indeed try to find the GAP code in that folder, and the real question is why the code hasn't been installed there during package installation.

comment:15 in reply to: ↑ 14 ; follow-up: Changed 21 months ago by SimonKing

Replying to SimonKing:

Replying to dimpase:

They appear to be a result of GAP files installed in a weird place, namely:

$SAGE_LOCAL/lib/python3.7/site-packages/$SAGE_LOCAL/share/sage/ext/gap/modular_cohomology/

It is indeed weird that $SAGE_LOCAL appears twice in this path. In the sources, if I see that correctly, this path appears only once, namely setup.py (data_files=[(os.path.join(SAGE_SHARE,"sage","ext","gap","modular_cohomology"))

I stand corrected: There are some places in the file auxiliaries.py, something like gap.Read(os.path.join(SAGE_SHARE,'sage','ext','gap','modular_cohomology','GapMaxels.g')). However, $SAGE_SHARE used only once in the path.

A clean solution would be to have a Sage or an environment variable giving the location of code of Gap extensions.

comment:16 follow-up: Changed 21 months ago by mkoeppe

For p_group_cohomology, see ticket #28711

Last edited 21 months ago by mkoeppe (previous) (diff)

comment:17 in reply to: ↑ 16 Changed 21 months ago by SimonKing

Replying to mkoeppe:

For p_group_cohomology, see ticket #28711

OK, I'll post fixes there.

comment:18 in reply to: ↑ 15 Changed 21 months ago by dimpase

Replying to SimonKing:

Replying to SimonKing:

Replying to dimpase:

They appear to be a result of GAP files installed in a weird place, namely:

$SAGE_LOCAL/lib/python3.7/site-packages/$SAGE_LOCAL/share/sage/ext/gap/modular_cohomology/

It is indeed weird that $SAGE_LOCAL appears twice in this path. In the sources, if I see that correctly, this path appears only once, namely setup.py (data_files=[(os.path.join(SAGE_SHARE,"sage","ext","gap","modular_cohomology"))

I stand corrected: There are some places in the file auxiliaries.py, something like gap.Read(os.path.join(SAGE_SHARE,'sage','ext','gap','modular_cohomology','GapMaxels.g')). However, $SAGE_SHARE used only once in the path.

A clean solution would be to have a Sage or an environment variable giving the location of code of Gap extensions.

You can call GAP and examine GAPInfo.RootPaths. When looking for a package, GAP appends pkg/ to each directory in this list, cf. 9.2 GAP Root Directories in its manual.

IMHO, Python wheels are build to be relocateable, so it's futile to try to install GAP data via setup.py. I'd say, install a GAP package separately, not from setup.py from spkg-install. By the way, GAP now has its own package manager. Maybe you can use it (https://github.com/gap-packages/PackageManager) - it's in gap_packages, I think.

comment:19 in reply to: ↑ 13 ; follow-up: Changed 21 months ago by mkoeppe

Replying to SimonKing:

Really unfortunate that the Cython code of p_group_cohomology has not been made optional Cython files in the Sage library, because then it would be easy to fix without upgrading the upstream source ball.

Yes, I would hope that we can make this improvement to the source structure in Sage 9.3. Similar to what has been done in the 9.2 cycle with giacpy_sage and sage_brial.

comment:20 in reply to: ↑ 10 Changed 21 months ago by SimonKing

Replying to dimpase:

Here are results of a local run, with 9.2.beta14 - that's newer than this ticket, but a bit older than the current rc - they match errors seen in the logs in the ticket description. They appear to be a result of GAP files installed in a weird place, namely:

$SAGE_LOCAL/lib/python3.7/site-packages/$SAGE_LOCAL/share/sage/ext/gap/modular_cohomology/

rather than $SAGE_LOCAL/share/sage/ext/gap/modular_cohomology/. So I see

Note that apparently $SAGE_LOCAL/share/sage is no more, it ceased to exist, it is an ex-folder.

comment:21 in reply to: ↑ 19 ; follow-up: Changed 21 months ago by SimonKing

Replying to mkoeppe:

Replying to SimonKing:

Really unfortunate that the Cython code of p_group_cohomology has not been made optional Cython files in the Sage library, because then it would be easy to fix without upgrading the upstream source ball.

Yes, I would hope that we can make this improvement to the source structure in Sage 9.3. Similar to what has been done in the 9.2 cycle with giacpy_sage and sage_brial.

Can you elaborate? At some point in the past, I suggested to change the p_group_cohomology spkg as follows:

  • Its Python and Cython code should go into the Sage library, the Cython files being optional extension modules.
  • Its .c files should remain an spkg, that also is the dependency of the optional extension modules.
  • I now suggest, as SAGE_LOCAL/share/sage has gone, to keep the .g files in the package, but install them into SAGE_LOCAL/share/p_group_cohomology

Do you suggest this, or something else?

It used to be a problem that the documentation of the optional Cython modules would not be built. Is there a way around?

EDIT: Is it possible to advise Sage's test suite to only run the tests of a particular file if a certain optional package is installed? Or is it still only possible for individual tests? I'd find it extremely awkward to mark each individual doctest as # optional: p_group_cohomology.

Last edited 21 months ago by SimonKing (previous) (diff)

comment:22 in reply to: ↑ 21 ; follow-up: Changed 21 months ago by mkoeppe

Replying to SimonKing:

Replying to mkoeppe:

Replying to SimonKing:

Really unfortunate that the Cython code of p_group_cohomology has not been made optional Cython files in the Sage library, because then it would be easy to fix without upgrading the upstream source ball.

Yes, I would hope that we can make this improvement to the source structure in Sage 9.3. Similar to what has been done in the 9.2 cycle with giacpy_sage and sage_brial.

Can you elaborate? At some point in the past, I suggested to change the p_group_cohomology spkg as follows:

  • Its Python and Cython code should go into the Sage library, the Cython files being optional extension modules.

Yes

  • Its .c files should remain an spkg, that also is the dependency of the optional extension modules.

Yes, in particular if that C library can also be used outside of Sage...

  • I now suggest, as SAGE_LOCAL/share/sage has gone, to keep the .g files in the package, but install them into SAGE_LOCAL/share/p_group_cohomology

You could either ship them with that spkg that provides the C library and then install it in SAGE_LOCAL/share/p_group_cohomology (or, more precisely, in the configured install prefix of your C library.

Or, if you want to ship it with sagelib, then (as I commented in #28711) these files should be put alongside the Python modules. sagelib and other Python packages have no business writing into $SAGE_LOCAL.

comment:23 in reply to: ↑ 22 ; follow-up: Changed 21 months ago by SimonKing

Replying to mkoeppe:

  • Its Python and Cython code should go into the Sage library, the Cython files being optional extension modules.

Yes

OK, that was what I have suggested in the past, but what was refused by other developers.

Crucial point, from my point of view: Documentation and doc tests, as explained in my other posts. Are these problems of optional extension modules solved?

  • Its .c files should remain an spkg, that also is the dependency of the optional extension modules.

Yes, in particular if that C library can also be used outside of Sage...

How do you define "can"? David Green, the original author of the c-code (that originally hasn't even been a library but only a couple of executables) has used them via command line, but I think he is the only one who ever did.

I'd say: I do not intend to ever publish the c-code outside of the Sage ecosystem.

  • I now suggest, as SAGE_LOCAL/share/sage has gone, to keep the .g files in the package, but install them into SAGE_LOCAL/share/p_group_cohomology

You could either ship them with that spkg that provides the C library and then install it in SAGE_LOCAL/share/p_group_cohomology (or, more precisely, in the configured install prefix of your C library.

That's what is currently happening.

Or, if you want to ship it with sagelib,

I don't.

then (as I commented in #28711) these files should be put alongside the Python modules. sagelib and other Python packages have no business writing into $SAGE_LOCAL.

comment:24 in reply to: ↑ 23 Changed 21 months ago by mkoeppe

Replying to SimonKing:

Replying to mkoeppe:

  • Its .c files should remain an spkg, that also is the dependency of the optional extension modules.

Yes, in particular if that C library can also be used outside of Sage...

How do you define "can"? David Green, the original author of the c-code (that originally hasn't even been a library but only a couple of executables) has used them via command line, but I think he is the only one who ever did.

This version of "can" is good enough.

I'd say: I do not intend to ever publish the c-code outside of the Sage ecosystem.

  • I now suggest, as SAGE_LOCAL/share/sage has gone, to keep the .g files in the package, but install them into SAGE_LOCAL/share/p_group_cohomology

You could either ship them with that spkg that provides the C library and then install it in SAGE_LOCAL/share/p_group_cohomology (or, more precisely, in the configured install prefix of your C library.

That's what is currently happening.

As long as you build this library using standard tools (preferably autotools) instead of Sage-specific scripts, and the C library does not depend on the Sage library, this is all fine.

comment:25 Changed 21 months ago by mkoeppe

  • Status changed from needs_info to needs_review

In any case, I don't think it's realistic to fix these issues in Sage 9.2. So let's use the present ticket to update the type of these two packages.

comment:26 follow-up: Changed 21 months ago by dimpase

I think a hotfix for p_group_cohomology, to install GAP files into the right place, using SAGE_LOCAL, despite being blasphemy in the Temple of Python, can be done very quickly.

Just add sdh_install call to spkg-install, which will put them where the package expects.

comment:27 Changed 21 months ago by dimpase

after doing ln -sf $SAGE_LOCAL/lib/python3.8/site-packages/$SAGE_LOCAL/share/sage/ext/gap/modular_cohomology $SAGE_LOCAL/share/sage/ext/gap/modular_cohomology (if it's python3.X, there should be python3.X there)

GAP files load, but Singular do not load

    sage.interfaces.singular.SingularError: Singular error:
       ? cannot open dickson.lib
       ? error occurred in or before STDIN line 15: `LIB "dickson.lib";`

So this is a similar to GAP files problem, only with Singular...

comment:28 in reply to: ↑ 26 Changed 21 months ago by SimonKing

Replying to dimpase:

I think a hotfix for p_group_cohomology, to install GAP files into the right place, using SAGE_LOCAL, despite being blasphemy in the Temple of Python, can be done very quickly.

Just add sdh_install call to spkg-install, which will put them where the package expects.

See my comment 39 at #28711: Apparently Sage prepends the absolute path os.path.join(SAGE_SHARE,"sage","ext","gap","modular_cohomology") with $SAGE_DESTDIR. Same problem with the Singular LIB.

So, I guess the real hotfix is to tell me: How should I provide a RELATIVE path in which to install the GAP respectively Singular code? Would it just work to write

  data_files=[ ( os.path.join( "share", "gap", "pkg", "modular_cohomology" ),
                 [ os.path.join( "pGroupCohomology", "GapMaxels.g" ),
                   os.path.join( "pGroupCohomology", "GapMB.g" ),
                   os.path.join( "pGroupCohomology", "GapSgs.g" ) ] ),
               ( os.path.join( "share", "singular", "LIB" ),
                 [ os.path.join( "pGroupCohomology","dickson.lib" ) ] ) ]

?

EDIT: I'd also need to change the places in auxiliaries.py, because currently the install path for the GAP helpers is SAGE_SHARE/sage/ext/gap/modular_cohomology, not SAGE_SHARE/gap/pkg/modular_cohomology. But SAGE_SHARE/sage has gone.

Last edited 21 months ago by SimonKing (previous) (diff)

comment:29 Changed 21 months ago by dimpase

can we get p_group_cohomology off the branch here?

comment:30 Changed 21 months ago by mkoeppe

  • Dependencies set to #28711
  • Status changed from needs_review to needs_work

comment:31 Changed 21 months ago by git

  • Commit changed from 402d45f5344ce17321164994d6e5eef7398f7819 to 674523cf0bc34368e2aa5effb280c0dcf71f9808

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4f5a810Upgrade p_group_cohomology to version 3.3.2
de8adfaRevert a change to "dependencies" of p_group_cohomology
cf60884Add upstream_url to p_group_cohomology
ae50157p_group_cohomology's upstream changed by some new commits
bd34c47build/pkgs/p_group_cohomology/checksums.ini: Fix upstream_url
245560dFix docbuild of p_group_cohomology
f4e5f6aFix a doctest in sage/tests/modular_cohomology.py
674523cbuild/pkgs/deformation/type: Downgrade to experimental

comment:32 Changed 21 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:33 Changed 21 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

ok

comment:34 Changed 20 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:35 Changed 20 months ago by vbraun

  • Branch changed from u/mkoeppe/downgrade_broken_optional_packages_to_experimental_for_sage_9_2 to 674523cf0bc34368e2aa5effb280c0dcf71f9808
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.