Opened 23 months ago
Closed 20 months ago
#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: |
Description (last modified by )
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
- Cc slelievre added
- Priority changed from critical to blocker
comment:2 Changed 22 months ago by
- Priority changed from blocker to major
comment:3 Changed 22 months ago by
- Priority changed from major to critical
comment:4 Changed 21 months ago by
- Branch set to u/mkoeppe/downgrade_broken_optional_packages_to_experimental_for_sage_9_2
comment:5 Changed 21 months ago by
- Commit set to 402d45f5344ce17321164994d6e5eef7398f7819
- Description modified (diff)
- Status changed from new to needs_review
New commits:
402d45f | build/pkgs/{deformation,p_group_cohomology}/type: Set to experimental
|
comment:6 Changed 21 months ago by
comment:7 follow-up: ↓ 8 Changed 21 months ago by
- Cc SimonKing added
Simon, what is the current status of your package?
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 21 months ago by
comment:9 in reply to: ↑ 8 Changed 21 months ago by
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: ↓ 14 ↓ 20 Changed 21 months ago by
- 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: ↓ 12 Changed 21 months ago by
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
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: ↓ 19 Changed 21 months ago by
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: ↓ 15 Changed 21 months ago by
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: ↓ 18 Changed 21 months ago by
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: ↓ 17 Changed 21 months ago by
For p_group_cohomology
, see ticket #28711
comment:17 in reply to: ↑ 16 Changed 21 months ago by
comment:18 in reply to: ↑ 15 Changed 21 months ago by
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: ↓ 21 Changed 21 months ago by
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
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: ↓ 22 Changed 21 months ago by
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
andsage_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
.
comment:22 in reply to: ↑ 21 ; follow-up: ↓ 23 Changed 21 months ago by
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
andsage_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: ↓ 24 Changed 21 months ago by
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
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
- 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: ↓ 28 Changed 21 months ago by
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
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
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 tospkg-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.
comment:29 Changed 21 months ago by
can we get p_group_cohomology off the branch here?
comment:30 Changed 21 months ago by
- Dependencies set to #28711
- Status changed from needs_review to needs_work
comment:31 Changed 21 months ago by
- Commit changed from 402d45f5344ce17321164994d6e5eef7398f7819 to 674523cf0bc34368e2aa5effb280c0dcf71f9808
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4f5a810 | Upgrade p_group_cohomology to version 3.3.2
|
de8adfa | Revert a change to "dependencies" of p_group_cohomology
|
cf60884 | Add upstream_url to p_group_cohomology
|
ae50157 | p_group_cohomology's upstream changed by some new commits
|
bd34c47 | build/pkgs/p_group_cohomology/checksums.ini: Fix upstream_url
|
245560d | Fix docbuild of p_group_cohomology
|
f4e5f6a | Fix a doctest in sage/tests/modular_cohomology.py
|
674523c | build/pkgs/deformation/type: Downgrade to experimental
|
comment:32 Changed 21 months ago by
- Status changed from needs_work to needs_review
comment:33 Changed 21 months ago by
- Reviewers set to Dima Pasechnik
- Status changed from needs_review to positive_review
ok
comment:34 Changed 20 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:35 Changed 20 months ago by
- 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
I'd say a broken optional package is by definition not a blocker...