Skip to content
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

[WIP] sage: 8.9 -> 9.2 #101447

Closed
wants to merge 1 commit into from
Closed

[WIP] sage: 8.9 -> 9.2 #101447

wants to merge 1 commit into from

Conversation

@omasanori
Copy link
Contributor

@omasanori omasanori commented Oct 23, 2020

Motivation for this change

This change updates SageMath to the latest version, migrates to Python 3, and gets rid of most of the current workarounds.

Depends on:

Part of #101964

TODO:

  • Make env-locations, sage-env and sage-with-env suitable with the new upstream scripts.
    • env-locations: patch src/bin/sage-env-config.in and build/pkgs/sage_conf/src/sage_conf.py instead (?)
    • sage-env: use upstream sage-env instead (?)
    • sage-with-env: TBD
  • Run tests and look at failures.
  • Update documentation if needed.
  • Check .nix files if more cleanup and/or simplification is possible.

This is work-in-progress, but any comments or suggestions are welcome!

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.
@@ -51,6 +52,8 @@
, libbraiding
, gmpy2
, pplpy
, sqlite # FIXME
Copy link
Member

@SuperSandro2000 SuperSandro2000 Oct 23, 2020

What needs to be fixed here?

Copy link
Contributor Author

@omasanori omasanori Oct 23, 2020

To determine where the dependencies come from. The dependency to gaic is explicitly mentioned in the official document:

The packages giacpy_sage and sage_brial have been merged into sagelib as sage.libs.giac and sage.rings.polynomial.pbori.

However, I have no idea about SQLite and Boehm GC (libgc). libgc especially smells; there might be a lack of build-time configuration or something. That is why I marked here.

Surely they are not clear to others, I will add some more words. Thank you for your comment!

@yurrriq
Copy link
Member

@yurrriq yurrriq commented Oct 25, 2020

I'm having trouble getting this to build locally, but here are a couple things I noticed so far.

  • /nix/store/fydrv699rcrsk8h35n79z2w8cm0iv2lx-sage-with-env-9.2.rc3/bin/sage: line 532: /nix/store/fydrv699rcrsk8h35n79z2w8cm0iv2lx-sage-with-env-9.2.rc3/bin/python3: No such file or directory -- I guess we need to run patchShebangs
  • remove sagenb from sagedoc.buildInputs (in pkgs/applications/science/math/sage/sagedoc.nix)

@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Oct 25, 2020

@yurrriq Thank you for your report! It helps me much.

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Oct 25, 2020

@omasanori Please rebase the PR on master to fix the eval django_2_2 borg eval error.

@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Oct 25, 2020

@SuperSandro2000 Done. Thanks!

@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Oct 26, 2020

Breaking news: SageMath 9.2 was just released! 🎉

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Oct 26, 2020

@omasanori Can you rebase again to fix undefined variable 'boolToString'? Sorry about the hassle.

@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Oct 26, 2020

No problem.

@omasanori omasanori force-pushed the sage-9.2 branch 2 times, most recently from 38d0d84 to 43b8201 Oct 27, 2020
@ofborg ofborg bot requested a review from peti Oct 27, 2020
@jcumming
Copy link
Contributor

@jcumming jcumming commented Oct 28, 2020

@omasanori ; this is awesome! Does the testsuite pass?

@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Oct 28, 2020

@jcumming Thank you for your interest!
No, it just compiles Cython code yet. I have been trying to adopt the Nix-specific scripts to newer upstream framework. Running the test suites is definitely the next step and I hope I will reach there soon.

@refnil
Copy link
Contributor

@refnil refnil commented Nov 20, 2020

While looking at the failing test stack trace, I saw that the test is calling libpynac which in turn is calling libpython2.7. For one, the appearance of python 2.7 seem suspicious. As you can see at the stack frame #4, the signal handler is called directly after the libpython2.7 call. I'll try to make pynac work with python 3.8.

#4  <signal handler called>
No symbol table info available.
#5  0x00007fff9cd7d4ab in PyInt_FromLong ()
   from /nix/store/xdha1qdqlh9hhymxq3w79zvjmwdyxqyz-python-2.7.18/lib/libpython2.7.so.1.0
No symbol table info available.
#6  0x00007fff9cfb4c87 in _GLOBAL__sub_I_numeric.cpp ()
   from /nix/store/jrdwhgc3dcrj8l9mxwh474ikkax89rh8-pynac-0.7.26/lib/libpynac.so.18
No symbol table info available.
#7  0x00007ffff7fe19ee in call_init ()
   from /nix/store/5didcr1sjp2rlx8abbzv92rgahsarqd9-glibc-2.32/lib/ld-linux-x86-64.so.2
No symbol table info available.

@refnil
Copy link
Contributor

@refnil refnil commented Nov 20, 2020

Moving pynac to python3 fixed to crash mentioned above. I tried opening sage repl and a jupyter notebook and both worked. However, it looked like some tests failed and one in particular hanged. Do you think we should add the commit to this PR or make a new PR for pynac? You can find the code there.

@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Nov 20, 2020

Excellent! Would you make a new dedicated PR for your changes, @refnil?
In this way, contributors can review the changes separately while I can cherry-pick them until it is merged to master.

@vale981
Copy link
Contributor

@vale981 vale981 commented Nov 24, 2020

Thank you for your efforts :).

I am a nix-noob, but would like to help to test this PR.
I would locally apply all associated PRs and then try to build the package. Is this how it's done, or is there a more efficient way?

@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Nov 24, 2020

Thank you for your interest, @vale981!
Ideally, each PR is independent and can be applied to master alone. This one had not been such a PR, but remaining one was merged some hours ago. Thus, you can try this PR by nix-shell -p nixpkgs-review --run "nixpkgs-review pr 101447" (unless you are using nixUnstable but you can refer to nix shell --help then 😉).

In general, nix-shell -p nixpkgs-review --run "nixpkgs-review pr <the ID of PR>" applies the PR on top of the current master branch pushed at NixOS/nixpkgs and builds everything it will affect: the packages, their dependencies if there are no cache files on cache.nixos.org and their reverse-dependencies (the packages depending on them, directly or indirectly). If it succeeds, you can test the resulting executables and so on.

See https://nixos.org/manual/nixpkgs/unstable/#chap-reviewing-contributions for more description.

@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Nov 25, 2020

So, thanks to helps from all of you, now sage-tests should run... with many failures.
They look not trivial (on my machine, the first one was an unhandled SIGFPE from an ECL-compiled library) but let's tackle with them.

@vale981
Copy link
Contributor

@vale981 vale981 commented Nov 25, 2020

I am getting:

builder for '/nix/store/c3i0kp4fazvi1d783f4pd2szwf0bfs9z-sagedoc-9.2.drv' failed with exit code 1; last 10 log lines:
      import sphinx.util.console
    File "/nix/store/s5x3vm0nshc74crkywlsbrxk161w7lm1-python3.8-sphinx-3.0.3/lib/python3.8/site-packages/sphinx/__init__.py", line 20, in <module>
      from .deprecation import RemovedInNextVersionWarning
    File "/nix/store/s5x3vm0nshc74crkywlsbrxk161w7lm1-python3.8-sphinx-3.0.3/lib/python3.8/site-packages/sphinx/deprecation.py", line 14, in <module>
      from typing import Any, Dict
    File "/nix/store/f57idsla85sxxgl9r5rfdzlgmqz4m0xj-python3.8-typing-3.7.4.3/lib/python3.8/site-packages/typing.py", line 1359, in <module>
      class Callable(extra=collections_abc.Callable, metaclass=CallableMeta):
    File "/nix/store/f57idsla85sxxgl9r5rfdzlgmqz4m0xj-python3.8-typing-3.7.4.3/lib/python3.8/site-packages/typing.py", line 1007, in __new__
      self._abc_registry = extra._abc_registry
  AttributeError: type object 'Callable' has no attribute '_abc_registry'

Looks like an incompatible python version.
Afterwards it stalls with: [1/0/4 built (1 failed)] building sage-tests-9.2 (installCheckPhase): [24 tests, 0.15 s].

@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Nov 25, 2020

@vale981 Thank you for testing!
The error in Sphinx seems well-known one with the typing package triggered and newer Python where there is built-in version of the library: python/typing#573. I will check if it is from sagedoc or sphinx.
Regarding sage-tests, it eats resources so much and takes hours so it is difficult to say it is a bug or usual run. My machine gets stuck at a different test...

@vale981
Copy link
Contributor

@vale981 vale981 commented Nov 25, 2020

Is it normal that there isn't any cpu activity for long stretches of time while running the tests?

@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Nov 25, 2020

I guess it is not, but I am not sure as I am also a new member trying to fix the package with help from veterans.

Any thoughts? @timokau

@timokau
Copy link
Member

@timokau timokau commented Nov 26, 2020

Regarding sage-tests, it eats resources so much and takes hours so it is difficult to say it is a bug or usual run. My machine gets stuck at a different test...

It might help to run the test in isolation.

Is it normal that there isn't any cpu activity for long stretches of time while running the tests?

No, at least I never noticed that. And I think I would have. If I had to guess, I'd say sage is waiting for some external process that isn't giving the expected reply. It uses pexpect to communicate with some dependencies, which can be brittle since it can depend on the exact formatting of the dependency's output.

You can re-enable timeouts in the test-suite in sage-tests.nix. It currently disables them by passing --timeout=0 to ensure deterministic/reproducible runs on slow or "overworked" machines. If you enable timeouts it should tell you which test is hanging.

@collares
Copy link
Member

@collares collares commented Nov 26, 2020

According to https://trac.sagemath.org/ticket/22191#comment:237, ECL needs to be patched to fix the SIGFPE crashes. The sage repository also has a few other ECL patches that were added after it was upgraded to version 20.4.24: https://git.sagemath.org/sage.git/log/build/pkgs/ecl/patches

@collares
Copy link
Member

@collares collares commented Nov 26, 2020

With this commit on top of the current PR, I get some "file not found" errors from the doctest harness and some failing tests but mostly the testsuite runs fine. Feel free to use anything from this commit.

Here's the full output of the test run. 11 files have failing tests. Based on timokau's suggestion, here's a command to only run tests from failing files, together with possible causes for the failures:

nix-build -E 'with (import ./. {}); sage.tests.override { files = [ 
"src/doc/en/reference/spkg/conf.py"  # Need to port https://git.sagemath.org/sage.git/patch/src/doc/en/reference/spkg?id=6257875c9aabcd730c3ba3801f9eb2e458d336d8
"src/sage/env.py"                    # checks if (SAGE_ROOT, SAGE_LOCAL) matches for subprocesses, but it is (None, ${sage-src}/src) in env.py (see sage-src.nix) and (${sage-src}, ${sage-with-env}) in sage (see sage-env.nix). See https://trac.sagemath.org/ticket/21707
"src/sage/features/__init__.py"      # tries to run the sage binary and fails
"src/sage/interfaces/tests.py"       # tries to run the sage binary and fails
"src/sage/tests/cmdline.py"          # tries to run the sage binary and fails
"src/sage/tests/startup.py"          # tries to run the sage binary and fails
"src/sage/libs/giac/giac.pyx"        # nixpkgs has giac 1.5.0-21, sage 9.2 uses 1.5.0-87
"src/doc/en/thematic_tutorials/sandpile.rst" # dict ordering (key 0 gets printed last)
"src/sage/sandpiles/sandpile.py"             # dict ordering (key 0 gets printed last)
"src/sage/symbolic/expression.pyx"   # https://trac.sagemath.org/ticket/30122
"src/sage/rings/rational.pyx"        # sage 9.2 uses gmpy2 2.1.0b5 (fixes https://github.com/aleaxit/gmpy/issues/237)
]; }'

The above command takes under 2 minutes to run on my old laptop, so testing should be fast.

@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Nov 26, 2020

@timokau Indeed.
@collares Thank you for your investigation! I overlooked that ticket. I hope I will be able to check this or next week end.

(Unlike European culture, December in Japan does not mean holiday but "work on last-minute tasks of the year, by the near-end of the year" thing...)

TODO: Write description here.

Signed-off-by: Masanori Ogino <167209+omasanori@users.noreply.github.com>
@collares
Copy link
Member

@collares collares commented Dec 1, 2020

An update: https://github.com/collares/nixpkgs/tree/sage-update has only three failing test files at the moment:

  • src/sage/interfaces/tests.py: tests for ecl's behavior when both stderr and stdout get redirected to /dev/full. I tried to apply write_error.patch (see commit) but it didn't fix the edge case.
  • src/doc/en/thematic_tutorials/sandpile.rst and src/sage/sandpiles/sandpile.py: for both files, the dict printing order is "wrong" (1, 2, 3, 0 instead of 0, 1, 2, 3) locally. I suspect some ordering problem causes the pretty printers for SandpileConfig and SandpileDivisors to not use the _sorted_dict_pprinter_factory('{', '}') pretty printer that was registered for dicts in src/sage/doctest/forker.py.

@omasanori If I fix the remaining failures before the weekend, is it OK for me to submit a new PR for review? You did most of the work and of course I would mention that in the PR. It would be completely understandable if you wanted to submit it yourself, so I am fine with waiting too.

@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Dec 1, 2020

No problem, please go ahead open a new PR. I will be truly glad to test and review your changes.
Kudos to @collares and everyone involved in!

@collares collares mentioned this pull request Dec 1, 2020
10 tasks
@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Dec 1, 2020

Superseded by #105615

@omasanori omasanori closed this Dec 1, 2020
@omasanori omasanori deleted the sage-9.2 branch Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants