Move sagemath-giac into sage.libs.giac (in-tree build)#41860
Conversation
|
Documentation preview for this PR (built with commit ad9cf4e; changes) is ready! 🎉 |
|
I think this will be easier for people who want to use giac interface. |
|
The ecl subproject is broken, now the git version is 26.3.26 not 24.5.10 @tobiasdiez |
|
Thank you very much for this, I was planning to do this myself. The (unsurprising) outcome of the split was This will also make sure that the giac integration backend is properly tested in every PR to prevent breakage. Nobody seems to be currently testing this backend. As for its usefulness: there are rather simple integrals that can only be solved with giac currently, such as |
|
Can you add giac to the conda environment files so that this is tested there? |
I will wait for the new version release, and make new conda-lock files |
|
I think that technically this is a step backwards to work around the fact that our Github organization is dysfunctional. It's much nicer not having to re-download, re-build, and re-install code that hasn't changed. I tagged a new release and have tried to update the tarball: https://github.com/sagemath/sagemath-giac/releases/tag/0.1.4 Giac still doesn't build on my machine, so I can't test it. The people who are able and willing to work on the giac interface should just have write access to sagemath-giac. I didn't get a notification about Antonio's issue/PR despite being the closest thing to a maintainer of the repo. In that situation I think you could request reviewers normally, and then if no one responds after a little while, be able to merge/release it yourself. A tag is really all it takes to make a release. There's no real harm in including a few extra files like |
Enabling giac support in our distro package now takes two hours longer than it used to, too. |
|
I have tested this in my local enviorment. |
And now, cypari2 is also unmaintained, it is hard to find someone to review the PR now. |
Besides the fact that I am not really willing to assume the burden of making releases, that is just really a small part of the problem. As I said, there is code in sagelib that is currently not being CI tested at all. Any PR may break it at any time, and it won't be detected until someone (probably me) runs the giac-dependant tests at a later time. Then I'd need to
All of this becomes a non-issue if the code is in sagelib. And I think a few extra MB to download is not a unreasonable price to pay in exchange. And with this being optional in meson, if someone doesn't want anything to do with giac (which I understand, being quite a hostile upstream), they can completely ignore it. |
Tested on Arch too, works as expected with or without giac. |
cypari2 is a different story: it is sage's upstream, so any breakage will affect sage and will eventually need to be dealt with. |
|
Please at least review the issue/PR where the interface was split. Giac bugs were causing CI failures and crashes in Volker's pre-release testing for years, so either you reintroduce those problems, or you leave giac disabled in the CI and you're no better off. Unlike most other projects we interface with, we have no ability to fix giac ourselves: there's no repo, no bug tracker, no mailing list, and the author is no help. Security issues go unaddressed forever. (I've been waiting over 10 years for the unsafe use of |
Most of sage is unmaintained, and we are all busy. I can review the cypari PR eventually but I need time to learn the pari API, and occasionally I have something better to do on a Sunday morning. This is a knee-jerk reaction that is going to replace a small problem (lack of admin bit on sagemath-giac) with a big one (giac itself being unstable and unfixable). |
I am certainly not asking nor expecting that we go back to compiling giac on CI. All I want is the sagelib giac depending code to be tested, which can be achieved by installing it in Conda (which already ships giac). All Volker was asking for is a way to disable giac, which is now in place (you actually have to actively enable it by having giac installed)
Again: the sagemath-giac non-maintenance is only one part of the problem. And I don't see how giac being unstable is going to be a big (or small) problem for anybody who doesn't want to use it and therefore doesn't enable it at build time. |
|
Setting as needs work awaiting for adding giac to conda environment. |
|
I think if someone does not need giac, do not install and directly build is OK. meson will skip this. this behavior like others optional packages. I think the benefit of this, for user, you just install giac from conda, then you can use this by rebuilding. |
The benefit is that it requires a two-hour rebuild of sagelib rather than a 2 minute build of sagemath-giac? |
Not that it's a good idea because the CI will randomly and unfixably crash, but you could accomplish the same thing without all of the other downsides by adding sagemath-giac to the CI. I would have done that myself... but we wanted the crashes to go away. |
|
meson's rebuilding only build the increasing targets. not the whole sagelib. it will be fast. and we do nit have sagemath-giac in conda package |
Only for developers with a git clone, end users have to rebuild the whole thing. |
|
Like, this is all covered on the original tickets... |
|
meson's build only relies on dependency graph and timestamp (mtime). and for non-develope users they might use conda install sage to directly use sage. but now we have giac as the sagelib's dependency in conda but without sagemath-giac as a conda package. it will make users confused that the giac is unavailable. |
Move the contents of the external sagemath-giac package (https://github.com/sagemath/sagemath-giac) directly into sage.libs.giac, building the Cython extension module in-tree via meson instead of relying on a separate pip package. This avoids the Cython namespace package limitations that originally forced the code into a separate sagemath_giac top-level package (cython/cython#5237, sagemath#5335, sagemath#6287). Changes: - Add giac.pyx, giac.pxd, context.py, gb.py, misc.h, auto-methods.pxi, keywords.pxi to src/sage/libs/giac/ - Add meson.build for giac Cython extension module - Add 'giac' option to meson.options - Update sage/libs/meson.build to build giac as a subdir - Update sage/features/sagemath.py (spkg='giac') - Update conftest.py, pyproject.toml, tools/update-conda.py to remove sagemath_giac external package references - Convert doctests from >>> to sage: format with optional -giac tag
- Add sage.libs.giac.giac to conftest.py skip list for ModuleNotFoundError - Guard __init__.py imports with try/except ImportError - Add giac.pyx to meson.build install_sources - Fix RST218 lint error in gb.py
5ffc5c0 to
ad9cf4e
Compare
|
I have added thw diaputed label now. |
|
@antonio-rojas maybe we need to open a vote for this? Seems it is disputed now. |
|
Let's not start this. We never needed the "disputed" tag before you-know-who, and we haven't needed it since. With that one exception, I am broadly capable of getting along with everyone who has ever worked on Sage. What are the benefits of this solution versus fixing the conda deps and adding sagemath-giac? There are many downsides (off the top of my head, but they are all listed somewhere):
On the other hand, the only problem I've heard with the current approach is that having the code in a separate repo makes bug fixes, releases, etc. more difficult. I completely agree. It's stupid that I, who cannot build giac, can commit directly to the repo, but you all can't. Every developer who uses giac should be an admin IMO. We are adults and can handle the review process without a release manager. Looking back at my sage-devel email, I requested this: The only other complaint is that we aren't testing the interface. But that really has nothing to do with where the code lives. We can test sagemath-giac just as easily as we can test |
|
Thanks. let more people discuss this. Thank you very much |
|
From a user's perspective, I can only see advantages for having a separate From a developer's perspective, I share the impression that essentially every repo in the sagemath repo is abandoned (except of course for the main |
|
lately it's primarily @cxzhong and @tobiasdiez who take care of problematic sagemath/ packages, I have too much on my plate(s) |
There is some Github magic on the other repos like https://github.com/sagemath/conway-polynomials to build and publish wheels to https://pypi.org/project/conway-polynomials/ when a new tag is pushed. If sagemath-giac builds now, that's all that's standing in the way. |
Well, the fact that it's only been used in a hostile context in the past doesn't necessarily imply it is a hostile procedure per se. I think it's a fine way to decide on disagreements. Of course a consensus is preferable, but not always possible.
There are exactly two distros shipping sagemath-giac according to repology. Speaking for 50% of them, I can say I'd very much welcome one less package to maintain (not changing often doesn't imply no maintenance: it needs to be rebuilt at least once a year for new python, with the added bonus that the dependency cycle with sagemath itself requires some additional bootstrapping).
The same can be said about all optional bits. I don't think this represents a significant percentage of the overall sage size.
Meson allows to easily make it opt-in only (and I assume if will be even easier when the explicit optional features PR is merged)
I can't remember the last time there was an ABI break in giac, compared to the high number of rebuilds needed for singular, flint, eclib,... updates. And this is only relevant for people who actually enable giac anyway. I'm not just arguing for the sake of it. I'm honestly struggling to understand the reason for so much resistance against this. To add one more point (besides what I've said so far) in favor of the merge: it is not benefitting from all the frequent linting/cleanup commits that some developers apply to sage code. Last time I had to look at it (for cython 3.2), the code was in quite bad shape, and it will only get worse with time in comparison.
Even though I asked for it myself in the past, I'm now wondering if that would actually be feasible - I assume sagemath-giac would pull in sagemath itself as a dependency in conda. How would one make sure that it doesn't interfere with the compiled one? @cxzhong maybe we can test this in a parallel PR when sagemath-giac is available in conda? |
We also have it in Gentoo, but it's in Francois's overlay, because the guy who commits to the main Gentoo repository can't build giac. I think this is the main point we disagree on. I love having one more package and would not have expected you to feel otherwise. Building sage takes a long time, but I can build these little extension packages quickly. If I were still able to build giac and were able to maintain sagemath-giac in Gentoo, it would save me hours upon hours of rebuilding compared to having in the SageMath tree. Having two packages means someone else (or nobody) can maintain the other one. Two packages isolates the ABI-related rebuilds so our end users don't have rebuild all of sage. Two packages means you can tweak the build options of one without affecting the other. Giac is a dud, but I also maintain a long list of other external programs and libraries that are used exactly nowhere outside of sage. Nobody else is using lcalc, symmetrica, palp, conway-polynomials, memory-allocator (etc.), and I love having those separate. It's just a much better design for our users and for me personally, since doing distro development usually involves pretending to be a user 10x over.
That all of them waste time and space isn't really a great argument for adding another, especially when the hard work has already been done to make it unnecessary. I'm not saying it's a huge improvement, but it is an improvement that you don't get the other way.
I don't want to go off on a tangent, but how are you determining when the ABI has broken? Upstream DGAF so in Gentoo our two options are (1) rebuild all consumers after every giac upgrade, or (2) never rebuild. The former never breaks but takes forever if sage.libs.giac is bundled with the rest of sage. The latter may work for a long time, but then one day the user will find that his sage crashes on startup. We generally prefer slow-but-working, so in this case having sagemath-giac be separate gives us the best of both worlds. The unspoken option (3) is for me to reverse engineer the changes to the source to look for ABI breaks, though this is not really on the table and is why I'm curious if you know a better way.
You're downplaying the benefits (it's not a lot of time/space, this only affects giac users, the ABI rarely breaks...), but no matter what weights you assign them, there's still a pretty long list of technical pros to the sagemath-giac approach that we don't get with the in-tree build. I weighed and discussed all of those benefits before splitting off the package. I invested a ton of time to make sure that we'd get them, and took the time to write a meson build system, and to fix the broken tools -- I certainly could have dumped another sagemath_bliss-style setuptools abomination on everyone and have saved myself two weeks. So for me is it was extra frustrating to see a PR to revert all of that for no given reason.
To avoid being hypocritical, I won't downplay this one. We need to rebase our interface in giacpy so that someone else maintains the bulk of it. This probably won't be easy at this point, though.
If you mess with the appropriate paths (or if they are already correct in the conda env) the build system for sagemath-giac should be able to find the existing sage. Why not test it on sage-the-distro though? I know it didn't build for a while, but we can fix that. |
|
after that, I may close this PR. I think it will be better to add sagemath-giac into conda then sagelib rely on this |
I completely agree with all that. But those are libraries independent from sage that could very well be used outside of it (whether it happens or not is irrelevant). You can't compare sagemath-giac with those - this is something completely dependant on sage, doesn't make sense without it. I could see the logic in splitting it if it were an add-on, but this is ripping out some middleware code that is still used in sagelib itself - in my mind it doesn't make logical sense.
I just check that it works in sage, which is the only package that uses it. I only care about ABI breakage that affects sage.
Please no - then we're back to all the issues caused by compiling giac in CI that we're trying to avoid to start with. And then someone needs to keep it up to date in sage-the-distro. Being the only one who apparently cares about it, guess who that would have to be (And deep inside I'm hoping for sage-the-distro to die in the not very distant future). |
|
@antonio-rojas what changes do you think are necessary to mitigate your pain-points if sagemath-giac stays an external package? |
|
I do not want to trace this PR now. |
At the very least it needs to be tested in conda in every PR like every other optional dependency. That won't solve the repo lack of maintenance though. |
|
I'm going out of town for the weekend and have jury duty on Monday, but I'll start a mailing list thread about the repos sometime next week. |
Move the contents of the external sagemath-giac package (https://github.com/sagemath/sagemath-giac) directly into sage.libs.giac, building the Cython extension module in-tree via meson instead of relying on a separate pip package.
This avoids the Cython namespace package limitations that originally forced the code into a separate sagemath_giac top-level package (cython/cython#5237, cython/cython#5335, cython/cython#6287).
Changes:
📝 Checklist
⌛ Dependencies