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

[MNT] unpin sphinx and plugins, with defensive upper bounds #4823

Merged
merged 4 commits into from Jul 9, 2023

Conversation

fkiraly
Copy link
Collaborator

@fkiraly fkiraly commented Jul 4, 2023

This PR replaces all the pins from sphinx and plugins with defensive upper bounds - next MAJOR release if the package is post-0, next MINOR release if the package is on zero-point.


original post: Diagnostic PR to see what happens if we remove all the pins from sphinx and plugins.

@fkiraly fkiraly added the maintenance Continuous integration, unit testing & package distribution label Jul 4, 2023
@fkiraly fkiraly changed the title DIAGNOSTIC: unpinned sphinx and plugins [MNT] DIAGNOSTIC: unpinned sphinx and plugins Jul 4, 2023
@yarnabrina
Copy link
Collaborator

Read the docs workflow seems to be working fine, so we can remove pins?

(Assuming other workflows are all independent of docs dependency, and hence don't matter.)

@fkiraly
Copy link
Collaborator Author

fkiraly commented Jul 4, 2023

Assuming other workflows are all independent of docs dependency, and hence don't matter.

Afaik this assumption is correct, I just checked the yml-s and could not find a docs set.

Read the docs workflow seems to be working fine, so we can remove pins?

Maybe upper bound to next major (or minor if zero-point), for safety?

@yarnabrina
Copy link
Collaborator

I doubt it'll have changes that'll break sktime, but as you said, it makes sense for safety. Let's do in this PR only.

(You can commit with [skip ci] in commit message I think, it'll skip all Github workflows, but read-the-docs one will still run. )

@fkiraly
Copy link
Collaborator Author

fkiraly commented Jul 4, 2023

You can commit with [skip ci] in commit message I think, it'll skip all Github workflows

Thanks for the reminder and learning moment!

Although in this specific case, I think it's worth to check carefully whether it has any interactions with the other dependencies by a way we don't foresee.

@fkiraly fkiraly changed the title [MNT] DIAGNOSTIC: unpinned sphinx and plugins [MNT] unpin sphinx and plugins, with defensive upper bounds Jul 4, 2023
@fkiraly fkiraly marked this pull request as ready for review July 4, 2023 14:38
yarnabrina
yarnabrina previously approved these changes Jul 4, 2023
Copy link
Collaborator

@yarnabrina yarnabrina left a comment

Choose a reason for hiding this comment

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

As long as read the docs workflow passes, I'm happy.

One minor non-blocking suggestion: use sphinx-issues instead of sphinx_issues as hyphen one is the PyPI name.

Similarly, Sphinx instead of sphinx. I don't think these differences matter though, as pip checks only for sanitised versions.

@fkiraly
Copy link
Collaborator Author

fkiraly commented Jul 4, 2023

One minor non-blocking suggestion: use sphinx-issues instead of sphinx_issues as hyphen one is the PyPI name.
Similarly, Sphinx instead of sphinx. I don't think these differences matter though, as pip checks only for sanitised versions.

Updated. I did not know that pip did some kind of closest name resolution.

I knew it doesn't allow package names which are too close to existing ones, e.g., only dashes or capitalization away.

@yarnabrina yarnabrina merged commit 5f37e4d into main Jul 9, 2023
24 checks passed
@yarnabrina yarnabrina deleted the sphinx-unpin branch July 9, 2023 04:18
fkiraly added a commit that referenced this pull request Jul 11, 2023
This PR updates the readthedocs env to the current [official
recommendations](https://docs.readthedocs.io/en/stable/config-file/v2.html),
namely python 3.11 and ubuntu 22.04. That should also, hopefully, speed
up the docs build.

Depends on #4823 to include the fix
for the `types` exception.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Continuous integration, unit testing & package distribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants