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

Unpin sphinx #25543

Merged
merged 1 commit into from Aug 18, 2023
Merged

Unpin sphinx #25543

merged 1 commit into from Aug 18, 2023

Conversation

oscarbenjamin
Copy link
Contributor

References to other Issues or PRs

Brief description of what is fixed or changed

Unpin Sphinx after #25542

This PR will not work until the upstream furo/Sphinx bug is fixed:

pradyunsg/furo#693

Other comments

Release Notes

NO ENTRY

@sympy-bot
Copy link

Hi, I am the SymPy bot. I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

  • No release notes entry will be added for this pull request.
Click here to see the pull request description that was parsed.
<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed

Unpin Sphinx after https://github.com/sympy/sympy/pull/25542

This PR will not work until the upstream furo/Sphinx bug is fixed:

https://github.com/pradyunsg/furo/discussions/693

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers. Formerly, `log(-x)` incorrectly gave `-log(x)`.

* physics.units
  * Corrected a semantical error in the conversion between volt and statvolt which
    reported the volt as being larger than the statvolt.

or if no release note(s) should be included use:

NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

@@ -806,4 +806,4 @@ These will give you the function parameters and docstring for

.. module:: sympy.simplify.simplify
.. autofunction:: powsimp
:noindex:
:no-index:

Choose a reason for hiding this comment

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

This won't work for Sphinx 7.1 and earlier, I would reccommend staying with noindex until your lower bound on Sphinx is 7.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is fine. According to sphinx-doc/sphinx#11533 :noindex: will be deprecated and removed at some point and I would rather just solve that problem now if I know about it.

Building docs is more of a development activity so we don't really need to support older versions of anything. Anyone who wants to do this can create a venv and use the requirements.txt. Ideally though we keep the versions unpinned and just make sure that the latest versions always work (CI always runs latest versions - the pin is only temporary).

Copy link
Member

Choose a reason for hiding this comment

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

A user, like me, may want to use an older sphinx. It is nice if it can run with past versions to be more flexible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I've restored :noindex:

Now this PR just unpins Sphinx so let's see if it passes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that it takes ~1hr to be able to see this.

Copy link
Member

Choose a reason for hiding this comment

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

@moorepants I'm not clear why you would want to use an older Sphinx. I can see if you might need an older Sphinx for some other projects, or if you have some unrelated dependencies that pin sphinx, but if that's the case you should just use a separate environment for the SymPy docs. Some of our docs dependencies might pin Sphinx (I know Furo tends to), but other than that, we only need to pin it for our own environment if if causes some issue that we haven't been able to fix.

Copy link
Member

Choose a reason for hiding this comment

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

When you are developing multiple packages (e.g. sympy + others) simultaneously it is preferable to use a single environment for that. When these packages depend on the same packages, then your environment can become unsolvable if some of the packages are too strict with dependency pins. Thus it is generally helpful to pin any dependencies (development or not) in the least restrictive possible way. Pinning to the absolute latest release of any dependency often causes issues for creating environments.

@github-actions
Copy link

github-actions bot commented Aug 17, 2023

Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with + are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with - are speedups.

Significantly changed benchmark results (PR vs master)

Significantly changed benchmark results (master vs previous release)

       before           after         ratio
     [8059df73]       [3183e048]
     <sympy-1.12^0>                 
-      87.3±0.4ms       56.5±0.3ms     0.65  integrate.TimeIntegrationRisch02.time_doit_risch(10)
-     6.96±0.02ms      3.78±0.01ms     0.54  logic.LogicSuite.time_load_file
-        2.10±0ms          650±2μs     0.31  polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'sparse')
-     10.3±0.02ms         1.94±0ms     0.19  polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'sparse')
-         367±1μs       81.0±0.2μs     0.22  polys.TimePREM_QuadraticNonMonicGCD.time_op(1, 'sparse')
-     4.81±0.01ms        362±0.9μs     0.08  polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'sparse')
-      11.8±0.7ms         1.09±0ms     0.09  polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'sparse')
-     6.26±0.01ms      3.95±0.01ms     0.63  polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(2, 'sparse')
-     27.2±0.04ms      12.1±0.02ms     0.45  polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'sparse')
-     6.94±0.03ms         1.17±0ms     0.17  polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(1, 'sparse')
-     16.2±0.03ms      9.30±0.02ms     0.58  polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(2, 'sparse')
-       210±0.2ms       71.4±0.2ms     0.34  polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'sparse')
-     6.39±0.04ms          530±1μs     0.08  polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'sparse')
-     27.4±0.04ms          853±2μs     0.03  polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'sparse')
-         610±1μs          195±1μs     0.32  polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(1, 'sparse')
-     6.44±0.01ms          201±1μs     0.03  polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'sparse')
-     17.0±0.04ms          205±1μs     0.01  polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'sparse')
-       162±0.2μs       89.5±0.3μs     0.55  solve.TimeMatrixOperations.time_rref(3, 0)
-       311±0.3μs        109±0.4μs     0.35  solve.TimeMatrixOperations.time_rref(4, 0)
-      31.7±0.1ms      13.7±0.09ms     0.43  solve.TimeSolveLinSys189x49.time_solve_lin_sys

Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).

@oscarbenjamin
Copy link
Contributor Author

The Sphinx build seems to be working here although it failed for me locally. Maybe I'm just getting confused about what versions of things are being used.

Ignore the CircleCI failure. I don't know how to restart CircleCI.

@oscarbenjamin
Copy link
Contributor Author

Okay, looks all good. Thanks @AA-Turner !

@asmeurer
Copy link
Member

I'm fine with only really supporting the latest Sphinx for the docs. Maybe we should add a lower bound in the requirements if we know some older version won't work.

@oscarbenjamin
Copy link
Contributor Author

Perhaps it would be good to freeze the versions at the time of creating a release branch and then have frozen versions in the requirements.txt in the sdist. That way you can go back to older release and still build the docs or if we need a bugfix release etc.

In any case this should be merged now.

@asmeurer asmeurer merged commit 1633cdb into sympy:master Aug 18, 2023
57 checks passed
@oscarbenjamin oscarbenjamin deleted the pr_sphinx_720 branch November 1, 2023 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants