-
Notifications
You must be signed in to change notification settings - Fork 227
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
setup: Upgrade pinned Sphinx version to v5.1.1 (Issue #492) #509
Conversation
I am happy to upgrade to v5.1.1. if the checks are passing, but I suggest that instead of removing the version requirement, we update it. I find that not fixing version numbers in dependencies (at least the major and minor number) is a bad practice and leads to issues like these(#492 and #507) happening. We can allow for patches to automatically install, and then regularly review dependency versions and upgrade them (there are extensions for this -- see Dependabot) |
Makes sense to me. I've updated the PR to pin to One concern with pinning dependencies too strictly is that someone who uses |
That's a fair point. In that case, we could pin only the major version of the packages. Alternatively (or also) have a GitHub workflow/action in place that checks for things breaking when new versions are released, and create a PR excluding that version (by `!=x.y.z`).
…On Mon, Aug 01, 2022 at 5:17 PM, Tom Tseng < ***@***.*** > wrote:
Makes sense to me. I've updated the PR to pin to sphinx~=5.1.1.
One concern with pinning dependencies too much is that someone who uses imitation
along with other libraries may get dependency conflicts, but I don't
expect issues with pinning packages for dev packages like sphinx.
—
Reply to this email directly, view it on GitHub (
#509 (comment)
) , or unsubscribe (
https://github.com/notifications/unsubscribe-auth/ABVWH36AFFOMRY3CPB7B4ODVW72BXANCNFSM55H5SLOQ
).
You are receiving this because your review was requested. Message ID: <HumanCompatibleAI/imitation/pull/509/c1201416680
@ github. com>
|
There's definitely a trade-off here in terms of greater reliability vs flexibility on versions. A compromise would be to pin packages that have caused us pain in the past. I think this is the first time Sphinx has broken for us, but we've had issues with flake8 and isort before. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. (Lint failure unrelated, docs build OK on RDT.)
Codecov Report
@@ Coverage Diff @@
## master #509 +/- ##
==========================================
+ Coverage 96.82% 96.85% +0.02%
==========================================
Files 82 82
Lines 7126 7126
==========================================
+ Hits 6900 6902 +2
+ Misses 226 224 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. |
Changes
We pinned Sphinx to v5.0.2 because v5.1.0 broke for us (issue #492). v5.1.1 fixes the problem, so we can upgrade Sphinx now.
Testing
Running
make html
indocs/
with Sphinx v5.1.1 doesn't crash:I didn't see anything obviously broken when clicking through some pages in the docs generated from this PR.