Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#27911 closed enhancement (fixed)

Do not restrict libgap.xxx to a predefined list

Reported by: nthiery Owned by:
Priority: major Milestone: sage-8.9
Component: interfaces Keywords:
Cc: vbraun, tscrim, dimpase, embray Merged in:
Authors: Nicolas M. Thiéry Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: d624b29 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (16)

comment:1 Changed 3 years ago by nthiery

  • Branch set to u/nthiery/do_not_restric_libgap_xxx_to_a_predefined_list

comment:2 follow-ups: Changed 3 years ago by nthiery

  • Commit set to d624b295cb99f5f7b2909b94d5bfd0560735c020

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


New commits:

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

comment:3 in reply to: ↑ 2 ; follow-up: Changed 3 years ago by dimpase

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')

comment:4 in reply to: ↑ 3 Changed 3 years ago by nthiery

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?

comment:5 Changed 3 years ago by nthiery

  • Cc embray added

comment:6 Changed 3 years ago by nthiery

  • Status changed from new to needs_review

comment:7 Changed 3 years ago by nthiery

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

comment:8 Changed 3 years ago by nthiery

  • Authors set to Nicolas M. Thiéry

comment:9 in reply to: ↑ 2 Changed 3 years ago by nthiery

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.

comment:10 Changed 3 years ago by nthiery

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.

comment:11 in reply to: ↑ 2 Changed 3 years ago by embray

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.

comment:12 Changed 3 years ago by nthiery

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

comment:13 Changed 3 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

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.

comment:14 Changed 3 years ago by vbraun

  • Branch changed from u/nthiery/do_not_restric_libgap_xxx_to_a_predefined_list to d624b295cb99f5f7b2909b94d5bfd0560735c020
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:15 Changed 3 years ago by embray

  • Commit d624b295cb99f5f7b2909b94d5bfd0560735c020 deleted
  • Summary changed from Do not restric libgap.xxx to a predefined list to Do not restrict libgap.xxx to a predefined list

comment:16 Changed 3 years ago by embray

  • Milestone changed from sage-8.8 to sage-8.9

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.

Note: See TracTickets for help on using tickets.