GAP: make GAP_ROOT_PATHS configuration unnecessary #37344

tornaria · 2024-02-14T17:56:22Z

As shown in #37308, running with GAP_ROOT_PATHS misconfigured results in awful breakage, and there are several situations where it's difficult to get this rigth (#37263, #37308 (comment)).

The current PR makes minimal changes in gap and libgap to make this configuration unnecessary. Instead, we obtain the necessary information from the gap command.

Note that the user can still change the gap command with SAGE_GAP_COMMAND. This includes the possibility to change the paths to a custom with something like SAGE_GAP_COMMAND='gap -l "path1;path2;path3"', etc.

Details:

interfaces/gap: remove __make_workspace, not used since 6be205b
interfaces/gap: cleanup gap_command()
interfaces/gap: replace global WORKSPACE by Gap() attribute

The first three commits are cleanup to replace global WORKSPACE by an attribute. This necessary to prevent circular references, and it's also good form and simplifies a number of tests which want running a gap instance with a temporary workspace file.

interfaces/gap: don't use GAP_ROOT_PATHS

This one is trivial since the gap command knows the paths.

gap: fast implementation of KERNEL_INFO()

Implements a fast version of KERNEL_INFO() which obtains the configuration we need by running the gap command but avoiding the full initialization of gap which is costly.

See #33446 (comment) and #33446 (comment) for this idea, which I discarded previously because it was pretty slow. Compare:

sage: from sage.interfaces.gap import KERNEL_INFO
sage: KERNEL_INFO._KERNEL_INFO__instance = None # force reload
sage: %time KERNEL_INFO()
CPU times: user 3.53 ms, sys: 1.04 ms, total: 4.57 ms
Wall time: 28.6 ms
{'KERNEL_VERSION': '4.12.2', 'GAP_ARCHITECTURE': 'x86_64-unknown-linux-gnu-default64-kv8', 'GAP_ROOT_PATHS': '/usr/lib64/gap/;/usr/share/gap/'}

with

sage: import subprocess
sage: %time subprocess.getoutput('gap -q --nointeract --bare -r -c \'Display(JoinStringsWithSeparator(KERNEL_INFO().GAP_ROOT_PATHS,";"));\'')
CPU times: user 0 ns, sys: 1.7 ms, total: 1.7 ms
Wall time: 790 ms
'/usr/lib64/gap/;/usr/share/gap/'

The implementation returns only a few selected variables from KERNEL_INFO() but it would be easy to add more variables as needed in src/sage/interfaces/gap_kernel_info.g.

TODO: add documentation and tests for KERNEL_INFO().

interfaces/gap_workspace: use KERNEL_INFO() to create filename
libs/gap/saved_workspace: use KERNEL_INFO() to compute gap timestamp
libgap: get GAP_ROOT_PATHS from KERNEL_INFO()

It is now very easy to rid of all usages of GAP_ROOT_PATHS, replacing it by use of KERNEL_INFO().

This even makes some code easier, e.g., gap_workspace_file() previously had to find and parse sysinfo.gap to read version and architecture, now it can obtain all the required information directly from KERNEL_INFO().

📝 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.

EDIT: formatting

Note that `self.__make_workspace` is true only when `use_workspace_cache` is false, hence `self.__use_workspace_cache and self.__make_workspace` is never true.

Since the `gap` binary itself knows the paths, there's no need. In fact, configure got the paths from running `gap`.

mkoeppe · 2024-02-14T21:05:13Z

I think the assumption that gap in PATH at runtime is the correct binary from which to extract this info is quite problematic.

This should really go through an Executable feature.

See:

Meta-ticket: Set environment for subprocesses invoked by Sage #30818

tornaria · 2024-02-14T22:14:20Z

I think the assumption that gap in PATH at runtime is the correct binary from which to extract this info is quite problematic.

We already assume that gap in PATH at runtime is the correct binary and it can be modified at runtime with SAGE_GAP_COMMAND as documented. With this PR changing SAGE_GAP_COMMAND will result in the correct GAP_ROOT_PATHS.

This should really go through an Executable feature.

Orthogonal to this PR. Please open a separate issue to avoid cluttering this one.

This PR is focused on making GAP_ROOT_PATHS unnecessary to avoid some types of misconfiguration, with as minimal change in code and behavior as possible.

Fixing other issues, unless caused by this PR, is not a goal.

mkoeppe · 2024-02-14T22:23:06Z

It's not orthogonal because it's the present PR would be a regression for library use of sage.libs.libgap, which currently works with the GAP as configured at compile time

tornaria · 2024-02-14T23:45:47Z

It's not orthogonal because it's the present PR would be a regression for library use of sage.libs.libgap, which currently works with the GAP as configured at compile time

I'd appreciate a more detailed report.

AFAICT the current version defaults GAP_ROOT_PATHS to $SAGE_LOCAL/lib/gap;$SAGE_LOCAL/share/gap which is wrong when gap is taken from system prefix (e.g. /usr) which is different from SAGE_LOCAL (e.g. when running sagemath-standard in a venv).

OTOH if using gap in PATH is a problem for sage.libs.libgap, it's also a problem for sage.interfaces.gap. The problem is then that SAGE_GAP_COMMAND is not being set correctly (not a regression since I didn't change anything about SAGE_GAP_COMMAND here).

Note that unlike gap command, the library does not know the paths [0]. The paths are taken from the gap command in PATH at configure time in the hope that they are the correct ones for the libgap that comes with -lgap. In particular this means that if SAGE_GAP_COMMAND is set to the gap command in PATH at configure time, this PR would not change the status quo. Does that sound ok?

There's still an issue that SAGE_GAP_COMMAND defaults to something non-trivial like gap -r -A <blah> maybe we really want to have just GAP_BIN = var("GAP_BIN", "gap") and configure GAP_BIN? While still giving the user the choice to override the gap invocation with SAGE_GAP_COMMAND.

[0] at best we could take the location of the library, say <PATH>/libgap.so, then look if <PATH>/gap/sysinfo.gap is available which should define a variable GAP which should point to the "right" binary, then use my code to get the info we want. Unfortunately, sysinfo.gap doesn't contain GAP_ROOT_PATHS either.

tornaria · 2024-02-15T02:38:57Z

@mkoeppe I added one more commit for GAP_BIN, does it look better this way?

If this is good for you, the missing bits are:

document KERNEL_INFO()
remove the AC_SUBST for GAP_ROOT_PATHS no longer used
add AC_SUBST for GAP_BIN to fix your issue

Otherwise, a wise man said: "describe the actual system configuration (not an imagined one), describe what happens". Maybe that would make it easier to work out. I already described explicitly two circumstances under which the status quo is broken and needs to be fixed.

orlitzky · 2024-02-15T13:23:24Z

Same comment as on #37308:

We append the SAGE_LOCAL path to the system one to support installing gap_packages as an SPKG with the system copy of GAP. Otherwise, you can ignore the SAGE_LOCAL path entirely outside of the sage-distribution. The (one) correct system path is already detected at build-time by ./configure. Tobias's meson branch moves that detection into sagelib itself.

tornaria · 2024-02-15T13:33:41Z

Same comment as on #37308:

We append the SAGE_LOCAL path to the system one to support installing gap_packages as an SPKG with the system copy of GAP.

Here's the relevant part of my comment in #37308. Let me know if this looks reasonable.

What we need is a configuration, let's tentatively call it SAGE_GAP_PKG which when set will be prepended to the system gap root paths. Is that correct?

This means:

a. for gap binary, we can run using -l "$SAGE_GAP_PKG;" where the empty last component means to prepend to system root paths:

$ gap -r -l "/SAGE/GAP/PATH;" -q -c 'Display(KERNEL_INFO().GAP_ROOT_PATHS);QUIT;'
[ "/SAGE/GAP/PATH/", "/usr/lib64/gap/", "/usr/share/gap/" ]

b. for libgap we can run using -l "$SAGE_GAP_PKG;<system-root-paths>":

$ ./libgap -r -l "/SAGE/GAP/PATH;/usr/lib64/gap/;/usr/share/gap/" -q -c 'Display(KERNEL_INFO().GAP_ROOT_PATHS);QUIT;'
[ "/SAGE/GAP/PATH/", "/usr/lib64/gap/", "/usr/share/gap/" ]

(here ./libgap is just a trivial shim to run gap-as-library)

c. the for libgap we can get from the gap binary:

$ gap -r --systemfile <(echo 'PRINT_TO("*stdout*",KERNEL_INFO().GAP_ROOT_PATHS,"\n");QuitGap();')
[ "/usr/lib64/gap/", "/usr/share/gap/" ]

(with the systemfile trick this is quick enough to do at runtime)

mkoeppe · 2024-02-15T16:53:34Z

What makes this a "blocker"?

tornaria · 2024-02-15T17:38:51Z

What makes this a "blocker"?

The same as #37178.

tobiasdiez · 2024-02-18T06:09:34Z

The conda workflow is failing here and these failures propagate to every other PR via the blocker label. Hence degrading the priority.

mkoeppe · 2024-02-18T06:40:20Z

@tobiasdiez Good call. I've seen these failures on PRs but didn't have a chance to check where they were coming from.

tornaria · 2024-02-19T01:10:47Z

The conda workflow is failing here and these failures propagate to every other PR via the blocker label. Hence degrading the priority.

Thanks for noticing it. I don't understand what's going on here:

...
  File "/home/runner/work/sage/sage/src/sage/interfaces/gap.py", line 262, in __init__
    gap_kernel_info = files(__package__).joinpath('gap_kernel_info.g')
  File "/usr/share/miniconda3/envs/sage/lib/python3.9/importlib/resources.py", line 147, in files
    return _common.from_package(_get_package(package))
  File "/usr/share/miniconda3/envs/sage/lib/python3.9/importlib/_common.py", line 14, in from_package
    return fallback_resources(package.__spec__)
  File "/usr/share/miniconda3/envs/sage/lib/python3.9/importlib/_common.py", line 18, in fallback_resources
    package_directory = pathlib.Path(spec.origin).parent
  File "/usr/share/miniconda3/envs/sage/lib/python3.9/pathlib.py", line 1082, in __new__
    self = cls._from_parts(args, init=False)
  File "/usr/share/miniconda3/envs/sage/lib/python3.9/pathlib.py", line 707, in _from_parts
    drv, root, parts = self._parse_args(args)
  File "/usr/share/miniconda3/envs/sage/lib/python3.9/pathlib.py", line 691, in _parse_args
    a = os.fspath(a)
TypeError: expected str, bytes or os.PathLike object, not NoneType
Error: Process completed with exit code 1.

In effect we are doing something like:

from importlib.resources import files
from sage.interfaces.gap import __package__
gap_kernel_info = files(__package__).joinpath('gap_kernel_info.g')

Is there a bug in that code? What do you get doing that in conda?

What would be the way to access package data files? I could place the file in sage/ext_data/gap/ but I thought the goal is to eventually move everything out of there.

I guess an alternative is to do os.path.join(os.path.dirname(__file__), 'gap_kernel_info.g').

tornaria · 2024-02-19T01:50:12Z

I think the issue is that importlib.resources does not support namespace packages in python 3.9 🤦

tobiasdiez · 2024-02-19T02:28:09Z

I think the issue is that importlib.resources does not support namespace packages in python 3.9 🤦

I agree. The runs for the other Python versions seem to work. Another reason to opt-out from the namespace packages for the moment #36753.

mkoeppe · 2024-02-19T02:33:55Z

People, that's why we carry the backport package importlib_resources.

github-actions · 2024-02-19T13:29:38Z

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

tornaria · 2024-02-19T14:47:25Z

People, that's why we carry the backport package importlib_resources.

importlib.resources is supported since python 3.7; importlib_resources is not used at all in sagelib, and can (should) be removed.

mkoeppe · 2024-02-19T17:22:36Z

People, that's why we carry the backport package importlib_resources.

importlib.resources is supported since python 3.7; importlib_resources is not used at all in sagelib, and can (should) be removed.

Yes, the module importlib.resources exists since python 3.7.

The backport package importlib_resources is maintained so that the features of newer Python versions can be used. https://pypi.org/project/importlib-resources/ offers a friendly version table.

More questions?

mkoeppe · 2024-02-19T17:35:32Z

importlib_resources is not used at all in sagelib

I know that -- I added it in #35203 because I knew that it will be needed later. And you just found the first use.

tornaria · 2024-02-19T17:45:39Z

The backport package importlib_resources is maintained so that the features of newer Python versions can be used. https://pypi.org/project/importlib-resources/ offers a friendly version table.

Not useful. This is unnecessary dependency; some distros don't ship importlib_resources anymore so we should stick to what's available in python. Besides, importlib.resources is in the stdlib which takes precedence over platlib so there's no way importlib_resources can override importlib.resources afaict.

More questions?

I did not ask a question, but since you knew python 3.9 did not support namespace packages, a mention before I had to debug this would have been nice.

mkoeppe · 2024-02-19T17:47:35Z

so there's no way importlib_resources can override importlib.resources

It doesn't try to. You use try: from importlib_resources import .... except: from importlib.reosurces import .....

mkoeppe · 2024-02-19T17:57:31Z

Yes, that's how the official backport packages work.

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

ince you knew python 3.9 did not support namespace packages, a mention before I had to debug this would have been nice

Sorry I didn't notice you are using importlib.resources in this branch!
But I haven't even really looked at your branch here yet, other than for checking my concern about "gap in PATH at runtime" before posting my comment #37344 (comment)

mkoeppe · 2024-02-19T18:15:25Z

some distros don't ship importlib_resources anymore

Could it be that these are distros that only ship Python packages for the latest version of Python?

mkoeppe · 2024-02-19T23:02:09Z

@tornaria Now that you know to use importlib_resources, how about correcting your comment #36753 (comment)?

tobiasdiez · 2024-02-22T14:50:30Z

@tornaria I'm sorry but I'm setting this back to "critical" as there are still issues coming from this (or another of the PRs labeled as blocker), as these are applied to PRs. We need to find a proper solution for this #37428.

vbraun · 2024-06-22T20:41:38Z

LGTM

orlitzky · 2024-06-22T22:52:49Z

LGTM

I don't think this is in a mergeable state, e.g.

class KERNEL_INFO(dict):
    """
    doc
    """

And while I would also love it if we didn't need sage_conf for GAP_ROOT_PATHS, I still hate to see a static piece of configuration become something that sage has to slowly infer whenever it starts.

vbraun · 2024-06-22T23:05:55Z

Its only instantiated on use, and not on Sage startup I presume?

Lets fix then docstring, then.

orlitzky · 2024-06-23T00:05:13Z

Its only instantiated on use, and not on Sage startup I presume?

I can't say for sure because the sage build fails for me right now and I don't have time to figure out why. Regardless, a huge portion of sage uses GAP, so it's not like you're going to avoid the penalty.

tornaria added 8 commits February 14, 2024 11:01

interfaces/gap: remove __make_workspace, not used since 6be205b

e892a85

Note that `self.__make_workspace` is true only when `use_workspace_cache` is false, hence `self.__use_workspace_cache and self.__make_workspace` is never true.

interfaces/gap: cleanup gap_command()

f836f8e

interfaces/gap: replace global WORKSPACE by Gap() attribute

bf0d4ae

interfaces/gap: don't use GAP_ROOT_PATHS

1b283ba

Since the `gap` binary itself knows the paths, there's no need. In fact, configure got the paths from running `gap`.

gap: fast implementation of KERNEL_INFO()

94cc1ff

interfaces/gap_workspace: use KERNEL_INFO() to create filename

01850d0

libs/gap/saved_workspace: use KERNEL_INFO() to compute gap timestamp

f8202af

libgap: get GAP_ROOT_PATHS from KERNEL_INFO()

f4ebc6b

tornaria requested a review from orlitzky February 14, 2024 17:56

tornaria added the s: needs review label Feb 14, 2024

gap: use GAP_BIN instead of hardcoded 'gap'

de8cd2b

tornaria added the p: blocker / 1 label Feb 15, 2024

tornaria mentioned this pull request Feb 15, 2024

Running gap with incorrect GAP_ROOT_PATHS results in awful error messages #37308

Open

tobiasdiez added p: critical / 2 and removed p: blocker / 1 labels Feb 18, 2024

tobiasdiez mentioned this pull request Feb 19, 2024

Readd init files #36753

Closed

5 tasks

sage.interfaces: make a package to avoid breakage in python 3.9

edcc082

tornaria added p: blocker / 1 and removed p: critical / 2 labels Feb 19, 2024

tobiasdiez added p: critical / 2 and removed p: blocker / 1 labels Feb 22, 2024

vbraun force-pushed the develop branch from eba5e19 to e5f42fa Compare June 3, 2024 22:15

vbraun added the s: positive review label Jun 22, 2024

vbraun added the s: needs work label Jun 22, 2024

kwankyu removed s: needs review s: positive review labels Jun 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GAP: make GAP_ROOT_PATHS configuration unnecessary #37344

GAP: make GAP_ROOT_PATHS configuration unnecessary #37344

tornaria commented Feb 14, 2024 •

edited

Loading

mkoeppe commented Feb 14, 2024 •

edited

Loading

tornaria commented Feb 14, 2024

mkoeppe commented Feb 14, 2024

tornaria commented Feb 14, 2024

tornaria commented Feb 15, 2024

orlitzky commented Feb 15, 2024

tornaria commented Feb 15, 2024

mkoeppe commented Feb 15, 2024

tornaria commented Feb 15, 2024

tobiasdiez commented Feb 18, 2024

mkoeppe commented Feb 18, 2024

tornaria commented Feb 19, 2024

tornaria commented Feb 19, 2024

tobiasdiez commented Feb 19, 2024

mkoeppe commented Feb 19, 2024

github-actions bot commented Feb 19, 2024

tornaria commented Feb 19, 2024

mkoeppe commented Feb 19, 2024

mkoeppe commented Feb 19, 2024

tornaria commented Feb 19, 2024

mkoeppe commented Feb 19, 2024

mkoeppe commented Feb 19, 2024

mkoeppe commented Feb 19, 2024

mkoeppe commented Feb 19, 2024

mkoeppe commented Feb 19, 2024

tobiasdiez commented Feb 22, 2024

vbraun commented Jun 22, 2024

orlitzky commented Jun 22, 2024

vbraun commented Jun 22, 2024

orlitzky commented Jun 23, 2024

GAP: make GAP_ROOT_PATHS configuration unnecessary #37344

Are you sure you want to change the base?

GAP: make GAP_ROOT_PATHS configuration unnecessary #37344

Conversation

tornaria commented Feb 14, 2024 • edited Loading

Details:

📝 Checklist

mkoeppe commented Feb 14, 2024 • edited Loading

tornaria commented Feb 14, 2024

mkoeppe commented Feb 14, 2024

tornaria commented Feb 14, 2024

tornaria commented Feb 15, 2024

orlitzky commented Feb 15, 2024

tornaria commented Feb 15, 2024

mkoeppe commented Feb 15, 2024

tornaria commented Feb 15, 2024

tobiasdiez commented Feb 18, 2024

mkoeppe commented Feb 18, 2024

tornaria commented Feb 19, 2024

tornaria commented Feb 19, 2024

tobiasdiez commented Feb 19, 2024

mkoeppe commented Feb 19, 2024

github-actions bot commented Feb 19, 2024

tornaria commented Feb 19, 2024

mkoeppe commented Feb 19, 2024

mkoeppe commented Feb 19, 2024

tornaria commented Feb 19, 2024

mkoeppe commented Feb 19, 2024

mkoeppe commented Feb 19, 2024

mkoeppe commented Feb 19, 2024

mkoeppe commented Feb 19, 2024

mkoeppe commented Feb 19, 2024

tobiasdiez commented Feb 22, 2024

vbraun commented Jun 22, 2024

orlitzky commented Jun 22, 2024

vbraun commented Jun 22, 2024

orlitzky commented Jun 23, 2024

tornaria commented Feb 14, 2024 •

edited

Loading

mkoeppe commented Feb 14, 2024 •

edited

Loading