Reviewing Code

All code that goes into Sage is peer-reviewed. Two reasons for this are:

  • Because a developer cannot think of everything at once

  • Because a fresh pair of eyes may spot a mathematical error, a corner-case in the code, insufficient documentation, a missing consistency check, etc.

Anybody (e.g. you) can do this job for somebody else’s PR. This document lists things that the reviewer must check before deciding that a PR is ready for inclusion into Sage.

You can now begin the review by reading the diff code.

Check the GitHub checks: We require all checks have passed.

Read the diff: Click “Files changed” tab of the PR. Read through the changes of all modified files. We use pull request reviews. You can add comments directly to changed lines.

Build the code: (This is optional if the build and test check has passed.) While you read the code, you can rebuild Sage with the new code. If you do not know how to download the code, see here.

The following should generally be checked while reading and testing the code:

  • The purpose: Does the code address the PR’s stated aim? Can it introduce any new problems? Does testing the new or fixed functionality with a variety of input, not just the examples in the documentation, give expected and robust output (and no unexpected errors or crashes)?

  • User documentation: Is the use of the new code clear to a user? Are all mathematical notions involved standard, or is there explanation (or a link to one) provided? Can he/she find the new code easily if he/she needs it?

  • Code documentation: Is the code sufficiently commented so that a developer does not have to wonder what exactly it does?

  • Conventions: Does the code respect Sage’s conventions? Python’s conventions? Cython’s conventions?

  • Doctest coverage: Do all functions contain doctests? Use sage -coverage <files> to check it. Are all aspects of the new/modified methods and classes tested (see Writing testable examples)?

  • Bugfixes: If the PR contains a bugfix, does it add a doctest illustrating that the bug has been fixed? This new doctest should contain the issue or PR number, for example See :issue:`12345`.

  • Speedup: Can the PR make any existing code slower? if the PR claims to speed up some computation, does the PR contain code examples to illustrate the claim? The PR should explain how the speedup is achieved.

  • Build the manuals: Does the reference manual build without errors (check both html and pdf)? See The Sage Manuals to learn how to build the manuals.

  • Look at the manuals: Does the reference manual look okay? The changes may have typos that allow the documentation to build without apparent errors but that may cause badly formatted output or broken hyperlinks.

  • Run the tests: Do all doctests pass without errors? Unrelated components of Sage may be affected by the change. Check all tests in the whole library, including “long” doctests (this can be done with make ptestlong) and any optional doctests related to the functionality. See Running Sage’s Doctests for more information.

For changes that affect the user interface, in particular, upgrades to IPython and Jupyter component packages, manual testing is crucial because our automatic tests do not cover the user interface. We recommend to use a Jupyter notebook with comprehensive tests of graphics and typesetting, some of which is Sage-specific.

You are now ready to change the PR’s status (see The status of a PR):

  • positive review: If the answers to the questions above and other reasonable questions are “yes”, you can set the PR to positive review status.

  • needs work: If something is not as it should, write a list of all points that need to be addressed in a comment and change the PR’s status to needs work status.

  • needs info: If something is not clear to you and prevents you from going further with the review, ask your question and set the PR’s status to needs info status.

  • If you do not know what to do, for instance if you don’t feel experienced enough to take a final decision, explain what you already did in a comment and ask if someone else could take a look.

For more advice on reviewing, see How to Referee Sage Trac Tickets (caveat: mercurial was replaced with Git and Trac with GitHub).

Note

“The perfect is the enemy of the good”

The point of the review is to ensure that the Sage code guidelines are followed and that the implementation is mathematically correct. Please refrain from additional feature requests or open-ended discussion about alternative implementations. If you want the code written differently, your suggestion should be a clear and actionable request.

Reviewing and closing PRs

PRs can be closed when they have positive review or for other reasons.

If a PR is closed for a reason other than positive review, use one of the resolution labels r: duplicate, r: invalid, r: wontfix, and r: worksforme. Add a comment explaining why the issue has been closed if that’s not already clear from the discussion.

If you think an issue has been prematurely be closed, feel free to reopen it.

Reasons to invalidate PRs

One Issue Per One Issue: An issue must cover only one issue and should not be a laundry list of unrelated issues. If an issue covers more than one issue, we cannot close it and while some of the patches have been applied to a given release, the issue would remain in limbo.

No Patch Bombs: Code that goes into Sage is peer-reviewed. If you show up with 80,000 lines of code bundle that completely rips out a subsystem and replaces it with something else, you can imagine that the review process will be a little tedious. These huge patch bombs are problematic for several reasons and we prefer small, gradual changes that are easy to review and apply. This is not always possible (e.g. coercion rewrite), but it is still highly recommended that you avoid this style of development unless there is no way around it.

Sage Specific: Sage’s philosophy is that we ship everything (or close to it) in one source tarball to make debugging possible. You can imagine the combinatorial explosion we would have to deal with if you replaced only ten components of Sage with external packages. Once you start replacing some of the more essential components of Sage that are commonly packaged (e.g. Pari, GAP, lisp, gmp), it is no longer a problem that belongs in our tracker. If your distribution’s Pari package is buggy for example, file a bug report with them. We are usually willing and able to solve the problem, but there are no guarantees that we will help you out. Looking at the open number of PRs that are Sage specific, you hopefully will understand why.

No Support Discussions: GitHub is not meant to be a system to track down problems when using Sage. An issue should be clearly a bug and not “I tried to do X and I couldn’t get it to work. How do I do this?” That is usually not a bug in Sage and it is likely that sage-support can answer that question for you. If it turns out that you did hit a bug, somebody will open a concise and to-the-point PR.

Solution Must Be Achievable: Issues must be achievable. Many times, issues that fall into this category usually ran afoul to some of the other rules listed above. An example would be to “Make Sage the best CAS in the world”. There is no metric to measure this properly and it is highly subjective.

The release process

It is good for developers and reviewers to be aware of the procedure that the Sage Release Manager uses to make releases. Here it is as of 2024:

Beta Release Stage: For preparing a new beta release or the first release candidate, all positively reviewed PRs with the forthcoming release milestone are considered. The Release Manager merges PRs in batches of 10 to 20 PRs. If a merge conflict of a PR to the Release Manager’s branch occurs, the PR is set back to “needs work” status by the Release Manager. (The author of the PR can try to guess which other PRs may be causing the conflict, make merge commits and declare them as dependencies, before setting back to “positive review” status. Alternatively, the PR author can wait until the next beta release and resolve the conflict then.) Each batch of merged PRs undergoes integration testing. If problems are detected, a PR will be set back to “needs work” status and unmerged. When a batch of PRs is ready, the Release Manager closes these PRs and proceeds to the next batch. After a few batches, a new beta release is tagged, pushed to the develop branch on the Sage repository on GitHub, and announced on sage-release.

Release Candidate Stage: After the first release candidate has been made, the project is in the release candidate stage, and a modified procedure is used. Now only PRs with a priority set to “blocker” are considered. PRs with all other priorities, including “critical”, are ignored. Hence if a ticket is important enough to merit inclusion in this stage, it should be set to “blocker” by adding p: blocker / 1 label.

Blocker PRs: The goal of the release process is to make a stable release of high quality. Be aware that there is a risk/benefit trade-off in merging a PR. The benefit of merging a PR is the improvement that the PR brings, such as fixing a bug. However, any code change has a risk of introducing unforeseen new problems and thus delaying the release: If a new issue triggers another release candidate, it can delay the release by up to 2 weeks. Hence developers should use “blocker” priority sparingly and should indicate the rationale on the PR. Though there is no one fixed rule or authority that determines what is appropriate for “blocker” status,

  • PRs introducing new features are usually not blockers – unless perhaps they round out a set of features that were the focus of development of this release cycle.

  • PRs that make big changes to the code, for example refactoring PRs, are usually not blockers.

Final Release: If there is no blocker PR for the last release candidate, the Release Manager turns it to the final release. It is tagged with the release milestone, and announced on sage-release.

Release management scripts are maintained in the repository sagemath/sage-release-management.