-
-
Notifications
You must be signed in to change notification settings - Fork 535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
deprecate pexpect interface to GAP #26963
Comments
comment:1
Note that there may be non-library code relying on the pexpect GAP interface. The current "official" version of p_group_cohomology is an example. For the next version 3.1, I am moving from pexpect to libgap. Actually I don't recall my original motivation for that change. However, from experience, I can tell (where GAP denotes that software and gap denotes the pexpect interface to GAP):
Concerning string evaluation: A complex piece of code in Sage would often involve tight loops in which myriads of temporary pexpect elements are created, manipulated, and deleted. Creation and deletion costs time; it is better to have one pexpect element With libgap, efficiency is no more a reason for using string evaluation. That's yet another reason why I think libgap is superior over gap. However, the work involved when switching to libgap may be a reason for other authors of third-party software to avoid the switch and keep using pexpect. EDIT, as I forgot the conclusion: Please do not remove pexpect too quickly. |
comment:2
Replying to @simon-king-jena:
That's why we have a deprecation process. It would be interesting to look at that package and see in what way it relies on GAP though.
Oh, well okay :)
You'd have to give an example.
Yep, that's probably the most annoying challenge. I'm not sure how deep that problem goes.
That's an arbitrary design "decision" though that seems to have been made without much reason. It doesn't need to be that way.
I'm not sure what you're talking about here and would need to see a concrete example. It sounds like you're describing a problem with the pexpect interface, in which case it's a moot point if it's going to be deprecated. But I'm probably not understanding you.
Again, hence, a deprecation period. I don't suspect it would be a massive overhaul for most uses but that's why I ask: I don't know for sure and would need to see examples.
The title of this ticket is "deprecate", not "remove". A follow-up "remove" ticket would only be made once deprecation is complete, and even then removal would not be for at least a year. |
comment:3
oops, sorry, ignore this one. |
comment:4
Replying to @embray:
See #26001, if you are interested. Note that the group cohomology package uses gap mainly when creating initial data of a cohomology ring and initial data of an induced homomorphism. So, the main part of the computations doesn't rely on GAP. The actual work is done with my code and with Singular (currently via pexpect).
That's interesting. I just tried to reconstruct it from an example that I met during my cohomology computations. But I find that even the evaluation of a string of length 1974754 is very quick. Have there been some recent improvement, in the past four months?
Now that the decision was made, it shouldn't be changed back, IMHO.
Indeed I was talking about ways of coding that used to be needed to overcome shortcomings of the pexpect interface. libgap doesn't have these shortcomings. However, it isn't a moot point: Translating the old work-around-pexpect-flaws code into make-the-best-out-of-a-C-library code is quite tedious, and more tedious than changing from 1-based to 0-based lists. The point only becomes moot since apparently you are not going to rip out pexpect from the next release. |
comment:5
Replying to @simon-king-jena:
That of course assumes an actual "decision" was made, which might be generous :) I'd rather any and all library interfaces in Sage be changed to actually have some consistency in their behavior. There are lots of nooks and crannies like this is Sage where, over the years, care has not been taken to ensure consistency in design. Better to fix it and for all, and also document some standard behavior that other interfaces should follow. This was done better, incidentally, in the case of the pexpect interfaces. Of course, doing this in a way that doesn't break backwards compatibility is tricky, and will require care. I'm not sure what the best answer is. |
comment:6
Looking at the p_group_cohomology sources (is there really no better way to get them then downloading the tarball through Sage??) I can see it would be a fair bit of work to do the conversion, though most of it should be doable mechanically. There's a lot of other stuff in there that could be written much more cleanly. Seems like it would be a good opportunity. |
comment:7
Replying to @embray:
As I have mentioned on various occasions, I already have changed p_group_cohomology from pexpect gap to libgap. See #26001, which according to Volker has a merge conflict (he didn't tell what it conflicts with). p_group_cohomology is a genuine SageMath package and cannot be used independently of SageMath. So, till today I didn't bother to link to the source tarball from my homepages, but I suppose that the package documentation can be found by google or duckduckgo. Anyway, my home pages provide links to the documentation and (since today) also to the source tarball of the package (version 3.1, hence, already with libgap). |
comment:8
Replying to @embray:
Not unless you have an excellent knowledge of regular expressions. Most of the transition was doable manually in two days or so. |
comment:9
I know how to search. I just meant the source tarball wasn't readily available anywhere else. Being an open source project I don't know why the repository isn't either. |
comment:10
Replying to @simon-king-jena:
I do. For changes that require more than a regexp the use of macros is also possible. So if you've already done the translation I don't know what the problem is. |
comment:11
Replying to @embray:
Simply I don't know how to make the repository public without paying for it. |
comment:12
Replying to @embray:
In comment:1, I think I made clear enough that I did not ask "how to change p_group_cohomology from gap to libgap". I was merely using p_group_cohomology as an example for non-library SageMath code. So, "what is the problem"? The problem from my perspective was that in the past most of p_group_cohomology's versions had to cope with backward incompatible changes in the SageMath API (where I define API to include all class definitions in .pxd files, all import locations and names in SageMath, and the infrastructure to build packages) and by changes in Singular. It has happened that I developed a new version coping with changes in SageMath, and while the new version was being reviewed new changes in SageMath forced me to change the package-being-reviewed yet again. Frankly, about one or two years ago I was so annoyed that I was very close to drop SageMath. So, the request that I was making in comment:1 boils down to: When doing non-trivial changes in Sage it isn't enough to fix the Sage library code. Keep in mind that there are people building code on top of SageMath (I suppose I am not the only developer who does). Make the transition easy for people and keep SageMath something that people can stably rely on --- or you'll lose them. |
comment:13
I'm sorry you had that experience but that's software development. Welcome to my life. Of course there are some legitimate issues you bring up here and I personally agree with everything you write with some nitpicks:
We're making this same mistake over again with GAP, but right now there isn't time to do it any other way. Of course, to some extent Sage can't be held responsible for changes to GAP that are not backwards compatible: Only the interfaces in Sage that use GAP are Sage's responsibility to keep working.
|
comment:14
Replying to @embray:
+1. When I started work in Sage (around 10 years ago), I had the impression that there is a realistic chance that when I contributed a bug fix it could be found in a release just 14 days later. I found that very encouraging.
Also +1. In older versions of p_group_cohomology, I was trying to maintain compatibility for several old Singular versions. I.e., I detected the version and used different code branches for each version, working around the bugs that I knew of in the different versions. But that was with the old spkg and thus I could envision people installing the latest p_group_cohomology version in an old Sage version. With the new Sage package structure, one can be virtually sure that a user cannot install the package in a Sage version older than the one for which the package was reviewed. |
comment:15
Replying to @simon-king-jena:
Github and gitlab are just for this, by default public and free. Create a git repo, with your code on github, say, make a release of the code (this is pushing 1 button :-)) |
Changed keywords from none to gap libgap |
comment:16
This ticket went off the rails a bit last time it was discussed. AFAICT Simon's p_group_cohomology now fully supports the libgap interface, and deprecating the pexpect interface will not have any major impact on it. I don't know how much other code is out there, if any, which makes heavy use of either interface. Nevertheless, I can add to the documentation a small guide for porting from pexpect gap to libgap--Simon's experience with this provided a number of helpful examples. I also don't propose removing the pexpect interface for now, just a roadmap for changing the |
This comment has been minimized.
This comment has been minimized.
comment:17
Replying to @embray:
That's right, I switched to libgap. So, p_group_cohomology is no obstacle against a deprecation of gap-via-pexpect. |
Once #26902 is complete, and Sage itself has no internal dependency on the pexpect interface to GAP, it should probably be deprecated entirely.
Completing work on #26902 will likely expose additional, previously unknown deficiencies in the libgap interface, but once those are all resolved there will be little (if any?) advantage to using the pexpect interface.
Here's a proposed strategy for deprecating the gap pexpect interface (henceforth referred to as "gapx" to avoid confusion) and eventually making the libgap interface the default gap interface:
Rename the global variable
gap
togapx
. I chose the namegapx
because it is reminiscent of "gap ex" and also "gap peXpect", but not "xgap" because that is too easily confused with the xgap interface.The
gap
global will, for now, still be the pexpect interface, but will produce a deprecation warning stating that in the next versiongap
will become thelibgap
interface, and also link to a code porting guide.After this deprecation period, change the
gap
global to be the libgap interface (keepinggapx
for the pexpect interface). Leavelibgap
as an alias forgap
but don't deprecate it.?. Do we do anything with the
_gap_
and_libgap_
magic methods? I think they should be renamed similarly_gap_
->_gapx_
but this could be harder to manage.Depends on #26902
Component: interfaces
Keywords: gap libgap
Issue created by migration from https://trac.sagemath.org/ticket/26963
The text was updated successfully, but these errors were encountered: