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

Update docs requirements #408

Merged
merged 5 commits into from
Jan 18, 2024
Merged

Conversation

BrianJKoopman
Copy link
Member

Prompted by Dependabot emailing me this advisory: https://github.com/simonsobs/sodetlib/security/dependabot/1

It wasn't clear to me why Jinja2 was pinned. I think we should update it, but please comment if this presents problems @jlashner.

This also follows the update that @msilvafe did the other day on socs in simonsobs/socs#618.

These were pinned previously due to some issue with the build on readthedocs
that has since been resolved. Pinning them is causing installations to drop
back two major versions in Sphinx to install (from the current 7.2.6 down to
5.3.0.)
@msilvafe
Copy link
Contributor

Prompted by Dependabot emailing me this advisory: Jinja vulnerable to HTML attribute injection when passing user input as keys to xmlattr filter

It wasn't clear to me why Jinja2 was pinned. I think we should update it, but please comment if this presents problems @jlashner.

This also follows the update that @msilvafe did the other day on socs in simonsobs/socs#618.

@mhasself expressed some issue with these updates as the newest sphinx versions are only 18 months old. I think he's worried it hasn't been tested enough and we may be introducing some bugs in that version. Maybe we can pin to the 2nd-most recent versions?

@jlashner
Copy link
Collaborator

I can't remember why I had to pin the Jinja version.... so I'm fine removing the pin and seeing if it still causes issues.

@BrianJKoopman
Copy link
Member Author

@mhasself expressed some issue with these updates as the newest sphinx versions are only 18 months old. I think he's worried it hasn't been tested enough and we may be introducing some bugs in that version. Maybe we can pin to the 2nd-most recent versions?

Understood, how about the solution in 466cbbe? Pinning to a version at the previously pinned level or higher?

A side-by-side comparison comparison of two builds looked OK to me, if anything with rendering improvements on the newer theme version.

I should also point out that the automatic deployment to simons1 broke with the MFA ssh requirements ~6 months ago, so the linked version in the README is out of date.

@mhasself
Copy link
Member

he's worried it hasn't been tested enough and we may be introducing some bugs in that version

No, I just don't like forcing people to bump dependencies needlessly. Sometimes it necessary, and that's fine. But sometimes people encounter a minor error and solve it by just pushing forward all the requirements, and that breaks people's installations (including on high inertia shared systems such as site computing, simons1, NERSC, della...). As a maintainer of some shared installations, I am going to resist dependency bumps somewhat.

Having looked into it a bit more ... seems that sphinx is pure python, and should be easy to upgrade on all our supported Python 3.x versions. So if it's fixing some problem, go for it.

Copy link
Collaborator

@jlashner jlashner left a comment

Choose a reason for hiding this comment

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

If you are able to build docs locally I am fine with merging this.

Separately, we need a PR that fixes docs servers. Now that sodetlib is public we shouldn't need a simons1 build anymore. A while ago I setup a readthedocs build: https://sodetlib.readthedocs.io/en/latest/, however builds have been failing since they require a readthedocs config file, so we should focus on making sure that's functional.

@BrianJKoopman
Copy link
Member Author

If you are able to build docs locally I am fine with merging this.

I reverted unpinning the theme and argparse plugin, and can build locally, but the workflow fails to build. If this is all irrelevant due to needing to move to readthedocs anyway, let me know how you want to proceed.

I'm sympathetic to the shared systems point, but do the docs requirements get installed on those systems?

@BrianJKoopman
Copy link
Member Author

Since it is fixing a build problem with the current workflow I'm going to keep it in. Will squash and merge now. I'll leave the readthedocs setup to you @jlashner.

@BrianJKoopman BrianJKoopman merged commit 32f4a8c into master Jan 18, 2024
1 check passed
@BrianJKoopman BrianJKoopman deleted the koopman/update-docs-requirements branch January 18, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants