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

Moving docs' requirements into the actual docs' requirements file #72

Merged
4 commits merged into from
Feb 10, 2023

Conversation

ghost
Copy link

@ghost ghost commented Feb 9, 2023

Reference Issue or PRs

This PR is not linked to an existing issue.

What does your PR implement? Be specific.

This PR moves the specific requirements of the documentation (matplotlib and gitpython) which are currently in the setup.py file to the docs/requirements.txt file, which is more appropriate as they are not used elsewhere.

@ghost ghost requested a review from maikia February 9, 2023 08:37
Copy link
Collaborator

@maikia maikia left a comment

Choose a reason for hiding this comment

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

@mandreux-owkin thanks for your contribution!
the cleaning up is a great idea, and it's true that gitpython and matplotlib should not be in setup.py if they are only used for docs.
but here it might be a bit more complex:
Sphinx is generating the docs, but we also use binder to run the example notebooks. (refer to .mybinder directory) There, in postbuild we are actually uninstalling all the docs/requirements to have a lighter build. I am afraid with this change the binder won't work. Feel free to suggest alternative solution.

@ghost
Copy link
Author

ghost commented Feb 10, 2023

@maikia thanks for your careful review. I checked the notebooks code and I am certain that gitpython is not used. We don't use any plot in the jupyter but I added matplotlib again to avoid breaking binder

Copy link
Collaborator

@maikia maikia left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @mandreux-owkin

@ghost ghost merged commit 37ef603 into main Feb 10, 2023
@ghost ghost deleted the maa/fix-docs-build branch February 10, 2023 13:51
This pull request was closed.
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.

2 participants