Skip to content

Comments

sage: 9.2 -> 9.3#116365

Merged
7c6f434c merged 11 commits intoNixOS:masterfrom
omasanori:sage-9.3
Apr 25, 2021
Merged

sage: 9.2 -> 9.3#116365
7c6f434c merged 11 commits intoNixOS:masterfrom
omasanori:sage-9.3

Conversation

@omasanori
Copy link
Contributor

@omasanori omasanori commented Mar 14, 2021

Motivation for this change

This upgrades SageMath to a newer version.

Tips: You can test only particular tests to save resources by the following expression:

# files: a list of files in the source code you want to test
nix-build -E 'with (import ./. {}); sage.tests.override { files = [
  "src/sage/geometry/polyhedron/base.py"
  "src/sage/matrix/matrix_double_dense.pyx"
]; }'

Candidates for new packages:

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 14, 2021
@omasanori omasanori requested review from 7c6f434c and timokau March 15, 2021 00:03
@omasanori
Copy link
Contributor Author

By the way, I would like to nominate @collares as a member of the Sage team if all of you @collares @timokau @7c6f434c agree, as @collares has tied up the loose ends on the 9.2 upgrade and worked actively on related packages during 21.05 development period.

@timokau
Copy link
Member

timokau commented Mar 15, 2021

Thank you for working on this.

Regarding the process: I think it might be easier to update to the different betas one by one.That way its easier to maintain a good version and spot the source of an issue. I used to keep the commits separate as well, since they might be useful later when you want to use git-bisect. In the end whatever works for you is best of course.

By the way, I would like to nominate @collares as a member of the Sage team if all of you @collares @timokau @7c6f434c agree, as @collares has tied up the loose ends on the 9.2 upgrade and worked actively on related packages during 21.05 development period.

I think it would be great if @collares wants to join.

@omasanori
Copy link
Contributor Author

Regarding the process: I think it might be easier to update to the different betas one by one.That way its easier to maintain a good version and spot the source of an issue. I used to keep the commits separate as well, since they might be useful later when you want to use git-bisect. In the end whatever works for you is best of course.

Agreed. I started from beta8 because we have already cherry-picked part of beta7, and I will stay at beta8 for a while. When the result of sage-tests become good enough, I will update to beta9, and then beta10, and so on. Thank you for your precious advice!

@collares
Copy link
Member

By the way, I would like to nominate @collares as a member of the Sage team if all of you @collares @timokau @7c6f434c agree, as @collares has tied up the loose ends on the 9.2 upgrade and worked actively on related packages during 21.05 development period.

Thanks for nominating me, @omasanori! I would be very happy to join and help with maintaining Sage.

@collares
Copy link
Member

I guess we need to add ecl to sagelib.nix's nativeBuildInputs.

@collares
Copy link
Member

collares commented Apr 4, 2021

I have tried to update this patch and a few dependencies, adapting @timokau's Singular update along the way. My changes are at https://github.com/collares/nixpkgs/tree/sage-9.3rc1 :) Feel free to use it as you like.

Currently failing tests:

  "src/sage/env.py"
  "src/sage/graphs/strongly_regular_db.pyx"
  "src/sage/interfaces/singular.py"
  "src/sage/libs/singular/function.pyx"
  "src/sage/plot/animate.py"
  "src/sage/plot/plot3d/base.pyx"
  "src/sage/repl/rich_output/backend_ipython.py"
  "src/sage/repl/rich_output/display_manager.py"
  "src/sage/schemes/elliptic_curves/ell_modular_symbols.py"
  "src/sage/schemes/elliptic_curves/ell_rational_field.py"
  "src/sage/symbolic/expression.pyx"

@omasanori
Copy link
Contributor Author

Thank you so much for your assistance, @collares!
Sorry for being inactive recently due to my employment situation. I will regain some spare time for NixOS in mid-May (unless something goes to worse...) so that I will be able to work on this PR.

@omasanori
Copy link
Contributor Author

Incorporated @collares's WIP branch into this PR. Thanks!

@collares
Copy link
Member

@omasanori Good luck with your job! I'll try to keep updating my branch. Currently I am having trouble with generating Singular documentation due to some segfaults (Sage 9.3 uses the singular.hlp documentation file).

@ofborg ofborg bot requested a review from collares April 13, 2021 01:14
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. labels Apr 13, 2021
@omasanori
Copy link
Contributor Author

omasanori commented Apr 13, 2021

I have not dug the problem yet, but I picked https://trac.sagemath.org/ticket/31592 (included in 9.3rc3) for testing.

Update: It still fails with **** Error: non-zero exit status of system call: 'echo '$' | ../Singular/Singular -teqr12345678 --no-rc ./examples/zeroRadical.sing > ./examples/zeroRadical.res': Inappropriate ioctl for device.

@collares
Copy link
Member

collares commented Apr 14, 2021

Pushed an update to my branch. Documentation-building now fails in intclToricRing when building normaliz_lib.tex, which uses the normaliz binary. So the next step is to package Normaliz, I think.

@collares
Copy link
Member

Ok, one more update on my branch. Singular can optionally use 4ti2, bertini, normaliz and topcom but does not require those packages. However, documentation-building (make singular.hlp is what I am testing, specifically) fails if those packages do not exist [0]. It would be good to package everything eventually, but (given that we never generated documentation before) I have disabled five sections of the manual that use the four missing packages for the time being. This way, we can parallelize the packaging work and still move forward with the Sage update.

[0] I believe this is a bug, because doc2tex.pl accepts arguments such as -exclude bertini which should skip sections, but that doesn't work for processing singular.doc. Apparently, the script handles the argument properly in HandleExample but there seems to be a bug in HandleLib (it forwards the tag when calling make recursively, but that does not lead to skipping the section).

@omasanori
Copy link
Contributor Author

It would be good to package everything eventually, but (given that we never generated documentation before) I have disabled five sections of the manual that use the four missing packages for the time being. This way, we can parallelize the packaging work and still move forward with the Sage update.

It makes sense to me. Thank you so much for figuring out the problem!

@collares
Copy link
Member

collares commented Apr 17, 2021

All tests are passing on x86_64! Well, unless you, like ofBorg, have a lot of CPUs (Singular/Singular#1086). The Singular bug is easy to patch but I'll wait a while before I patch it myself.

I downgraded eclib because, weirdly, Sage 9.3 testsuite checks (in https://github.com/sagemath/sage/blob/develop/src/sage/schemes/elliptic_curves/ell_modular_symbols.py#L305) that eclib contains a certain bug. "Problem" is, the demonstrated bug is not present in eclib 20210223 anymore :) I can also just remove the offending line from the test.

There's something weird happening when building Singular on aarch64 but I don't have a machine to debug it. Help wanted :) The error is:

checking whether the C compiler works... no
configure: error: in `/build/source/omalloc':
configure: error: C compiler cannot create executables
See `config.log' for more details
configure: error: ./configure failed for omalloc
error: builder for '/nix/store/6p796arxk77cpblx28h72b2cbdk86xs7-singular-4.2.0p2.drv' failed with exit code 77;

@collares
Copy link
Member

collares commented Apr 20, 2021

Earlier today I realized that Sage 9.2 is broken on master (four tests, some/all using Singular, produce different results). So instead of debugging that, I tied up some loose ends on my branch. With today's chagnes, I believe the Sage 9.3 upgrade is ready for review.

If it gets reviewed before 21.05, I think we should include it into the May release. Sage doesn't really change much between the RC stage and the final release, and we wouldn't have to chase what changed in master (but if I had to guess, I would guess the FLINT upgrade).

@collares
Copy link
Member

collares commented Apr 22, 2021

@omasanori Really sorry to cause more work for you, but it seems that I needed to revert ax_prog_cxx_for_build.m4 too, since now there seems to be a problem with linker flags for C++ compilation (it doesn't fail when running configure, but that's probably just luck). collares@bb3b98e replaces the last commit. Thanks :)

collares and others added 6 commits April 22, 2021 09:01
Adds proper tests. Also removes the "enableFactory" option because
singular actually enables factory by default and explicitly disabling it
breaks the build. So the option was never really available.
@omasanori
Copy link
Contributor Author

No worries, and thank you for your tireless work, @collares!

@collares
Copy link
Member

collares commented Apr 22, 2021

Now it freezes while building documentation. The last log line is for algebra.lib, but that's complete, so I assume it hangs in assprimeszerodim.lib, which is where we were getting vspace segfaults before. I am tempted to just disable vspace on aarch64 to see what happens: collares@6aafc7a

Some information: "vspace is a C++ library designed to allow processes in a multi-process environment to interoperate via mmapped shared memory. The library provides facilities for shared memory allocation and deallocation, shared mutexes, semaphores, queues, lists, and hash tables." Disabling vspace disables concurrency in a small amount of Singular libraries. Those libraries will still work with no change in functionality.

@omasanori
Copy link
Contributor Author

Thanks to @collares' dedicated effort, finally it passes all tests before the ZHF period for 21.05! We can still wait for the 9.3 GA for ~1.5 weeks, though.
Any opinions @timokau?

@collares collares removed their request for review April 22, 2021 14:20
@collares
Copy link
Member

collares commented Apr 25, 2021

To be honest I would be happy to see this land soon. Sage 9.2 is broken on master and this is starting to affect other PRs (I am glad @7c6f434c pointed #120614 out, for example). c12ef05 would need to be reverted before landing, though (Edit: I created #120648 for that).

@7c6f434c The only changes here since you last reviewed things are the three new Singular commits, all of them aimed at fixing aarch64 (it worked!).

@7c6f434c 7c6f434c merged commit 14f65f0 into NixOS:master Apr 25, 2021
@omasanori omasanori deleted the sage-9.3 branch April 26, 2021 00:47
@omasanori
Copy link
Contributor Author

Thank you all for your excellent work!

Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work! Sorry for being a bit unresponsive.

I left a few post-merge comments. No big issues though. I hope we can update to a released version before the branch-off. That seems likely with the current schedule.

Thank you for keeping sage working and up to date :)

pname = "threejs-sage";
version = "r122";

src = fetchFromGitHub {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to build this from the upstream sources. This is probably more practical though, given the node packaging difficulties.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, we can and should build it from upstream sources! It was my original plan and I think I know how to do it (stealing a few tricks from the rust-analyzer VS Code extension package). I just didn't find the time to finish it and thought this would be a good enough stopgap given that two users complained about it (one on IRC, one in #118806) :) I'll have some more time next month to do this. But I think we would probably need to pin to a specific version of upstream threejs, since they're doing a ton of refactorings to use ES6 Modules and it would be too much of a moving target.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good. Feel free to ping me if you end up working on it. Don't stress yourself about it though :)

fetchSubmodules = true;
};

patches = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these patches and the sed line in postPatch are not nixos specific right? Are there corresponding upstream issues?

Copy link
Member

@collares collares Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agreed. Here are the things I need to do:

I will tick those boxes as I do those items.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually @timokau, if you do have an aarch64 machine available, running Singular tests (removing the disable-vspace.patch patch) under gdb to diagnose vspace-related problems would be great. I don't think upstream will be able to solve the bug with the information I have.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going above and beyond, as always :)

I do have access to the community build box. My time for tinkering on open source things is unfortunately limited recently (it's always a question of priorities of course, but other things rank higher for me right now). I could run some tests for you, but digging into the hanging issue would probably require quite some time in the debugger.

Maybe upstream would already appreciate a report of the issue. They might consider applying the "disable vspace" workaround, even if they don't want to invest much time into aarch64 support.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense! I will file a bug upstream, then I can debug later (perhaps asking for temporary access to the community build box). Thanks for the suggestion :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Feel free to mention me or link this conversation if you request access to the community build box. We have only interacted in a few GitHub PRs so far, but those interactions were all positive. You and @omasanori have taken over sage maintenance and I think the nixpkgs community is lucky to have you. Establishing trust over the internet is hard, but maybe this helps a bit. Thanks!

@collares
Copy link
Member

Good work! Sorry for being a bit unresponsive.

I left a few post-merge comments. No big issues though. I hope we can update to a released version before the branch-off. That seems likely with the current schedule.

Thanks for the review and for the comments! Next time I do a Sage PR I will add the pynac comment and change the Singular forum URL at least.

I also hope we can update to the final release before the branch-off. But if it doesn't work out we can always backport the update to 21.05, since Sage probably won't change much.

@omasanori
Copy link
Contributor Author

Good work! Sorry for being a bit unresponsive.

No problem, @timokau! Your well-grounded work makes this possible. Thank you for your insightful review comments.

I also hope we can update to the final release before the branch-off. But if it doesn't work out we can always backport the update to 21.05, since Sage probably won't change much.

Absolutely.

@omasanori
Copy link
Contributor Author

SageMath 9.3rc5 was tagged several hours ago with 7 positive-review tickets remaining.

@timokau
Copy link
Member

timokau commented May 11, 2021

Good work! Sorry for being a bit unresponsive.

No problem, @timokau! Your well-grounded work makes this possible. Thank you for your insightful review comments.

:)

I also hope we can update to the final release before the branch-off. But if it doesn't work out we can always backport the update to 21.05, since Sage probably won't change much.

Absolutely.

For reference (thanks @collares): #122592

@timokau timokau mentioned this pull request May 11, 2021
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants