Do not restrict libgap.xxx to a predefined list

[nthiery]

Currently libgap.xxx fails if xxx is not in one of the two hard coded lists common_gap_functions or common_gap_globals. This has the following consequences:

• Additional maintenance burden: Sage needs to be updated whenever GAP introduces a new useful global function/object.
• This is arbitrarily preventing expert users from accessing functions they may deem interesting
• More important: if a user installs additional packages, he can't access its functions via libgap.xxx.

Proposal: resort to libgap.eval("xxx") if xxx is not in the predefined lists. In fact, it seems that we could always resort to libgap.eval("xxx"): tests seems to be passing as well with the attached branch, and the code is simpler.

CC: @vbraun @tscrim @dimpase @embray

Component: interfaces

Author: Nicolas M. Thiéry

Branch: d624b29

Reviewer: Volker Braun

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

[nthiery]

Branch: u/nthiery/do_not_restric_libgap_xxx_to_a_predefined_list

[nthiery]

comment:2

Does anyone remember the original motivation for having a fixed list? For special casing gap functions?

---

New commits:

d624b29

27911: Do not restric libgap.xxx to a predefined list

[nthiery]

Commit: d624b29

[dimpase]

comment:3

Replying to @nthiery:

Does anyone remember the original motivation for having a fixed list? For special casing gap functions?

for tab-completions, perhaps?

I'd rather see libgap.xxx becoming, if needed, libgap.function_factory('xxx')

[nthiery]

comment:4

Hi Dima!

Thanks for the quick feedback.

for tab-completions, perhaps?

Yeah, I can indeed see the motivation of presenting a trimmed down tab-completion list for users. Although (but that's a different discussion) I would tend to consider that we should not be maintaining such a list ourselves, but rather somehow fetch it from GAP: they know better!

But it's one thing not to advertise, and another to prevent access.

I'd rather see libgap.xxx becoming, if needed, libgap.function_factory('xxx')

I am not sure what you mean. We agree that, if GAP or some GAP package defines some important top level entry point Foo, then we want it to be accessible as libgap.Foo, right?

[nthiery]

comment:7

All tests passed here (up to a presumably unrelated timeout in src/sage/manifolds/differentiable/tensorfield.py.

[nthiery]

Author: Nicolas M. Thiéry

[nthiery]

comment:9

Does anyone remember the original motivation for having a fixed list? For special casing gap functions?

Looking back in time, the fixed list was introduced in #19917. Handling package loading was postponed to a later ticket.

[nthiery]

comment:10

For the record: before this change:

sage: %timeit libgap.Group
The slowest run took 2114820.50 times longer than the fastest. This could mean that an intermediate result is being cached.
1 loop, best of 3: 954 ns per loop
sage: %timeit libgap.Group
The slowest run took 9.65 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 618 ns per loop
sage: %timeit libgap.Group
The slowest run took 23.93 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 628 ns per loop
sage: %timeit libgap.Group
The slowest run took 9.45 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 631 ns per loop

After the change:

sage: %timeit libgap.Group
The slowest run took 118578.25 times longer than the fastest. This could mean that an intermediate result is being cached.
1 loop, best of 3: 1.91 µs per loop
sage: %timeit libgap.Group
The slowest run took 9.47 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 629 ns per loop
sage: %timeit libgap.Group
The slowest run took 22.15 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 624 ns per loop

So no noticeable influence on tight loops involving libgap.Foo access.

[embray]

comment:11

Replying to @nthiery:

Does anyone remember the original motivation for having a fixed list? For special casing gap functions?

Here's the sage-devel thread where I asked this: https://groups.google.com/d/msg/sage-devel/iPTfFXUk8XU/UX3qr42xAQAJ

As Volker noted (more importantly for #27923) part of the issue is that building a full list for tab-completion is slow. I wonder if that couldn't be sped up though through various things like caching, and smarter search techniques. Though I also have to wonder why getting a list of global variable names from GAP would be so slow in the first place. Even with some ~10000 entries it shouldn't take that long. A similarly-sized set of global variables on Python does not require as much overhead. Might be worth looking into on the GAP side if there's anything that an be done.

[nthiery]

comment:12

Ok. Thanks for the feedback. So this is all about #27923 (tab completion). I gather that, for this ticket (attribute access), there is no unremembered reason to restrict to a fixed list. Good. Let's go for it then.

Can someone review the current implementation?

Thanks,
Nicolas

[vbraun]

comment:13

I think building the autocomplete list is slow because gap is grinding though of its files; Just sending a bunch of strings over to Sage is not going to be noticeable. Also it tends to cough up a lot of package-private functions in GAP that necessarily are in the public namespace but users aren't really supposed to access.

[vbraun]

Reviewer: Volker Braun

[vbraun]

Changed branch from u/nthiery/do_not_restric_libgap_xxx_to_a_predefined_list to d624b29

[embray]

Changed commit from d624b29 to none

[embray]

comment:16

Not in Sage 8.8. Let's please to try keep tickets' milestones related to the release in which we actually intend to include them, and in particular the release in which they were actually included, especially when closing tickets.