#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: |
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
- Branch set to u/nthiery/do_not_restric_libgap_xxx_to_a_predefined_list
comment:2 follow-ups: ↓ 3 ↓ 9 ↓ 11 Changed 3 years ago by
- Commit set to d624b295cb99f5f7b2909b94d5bfd0560735c020
comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 3 years ago by
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
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
- Cc embray added
comment:6 Changed 3 years ago by
- Status changed from new to needs_review
comment:7 Changed 3 years ago by
All tests passed here (up to a presumably unrelated timeout in src/sage/manifolds/differentiable/tensorfield.py
.
comment:8 Changed 3 years ago by
comment:9 in reply to: ↑ 2 Changed 3 years ago by
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
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
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
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
- 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
- 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
- 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
- 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.
Does anyone remember the original motivation for having a fixed list? For special casing gap functions?
New commits:
27911: Do not restric libgap.xxx to a predefined list