-
Notifications
You must be signed in to change notification settings - Fork 135
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
Add Sphinx to build documentation for DLPack #90
Conversation
This PR introduces Sphinx as a documentation tool for DLPack. It uses the Breathe extension to parse Doxygen output (because Sphinx doesn't directly parse C/C++). The new documentation uses the PyData sphinx theme (adopted by several projects in the data science community). To keep the diff minimal, I didn't change the default configuration but it can be changed later to add sidebars, logo, social media links, etc. I have deployed the documentation [here](https://tirthasheshpatel.github.io/dlpack/) for testing. In a follow-up PR, we can add a script that deploys the docs on `gh-pages` branch of the main repo. Then we can start populating the docs with examples, tutorials, list of projects that use DLPack, etc. This PR also adds a small CI script to build docs (just to test Make and CMake changes work properly).
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.
Some comments to help reviews get started.
@@ -0,0 +1,3 @@ | |||
Sphinx==4.4.0 | |||
pydata-sphinx-theme==0.7.1 |
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.
The latest version (0.8.0) doesn't work well with older versions of Jinja (Sphinx dependency). xref pydata/pydata-sphinx-theme#564. So, pin to the older version for now.
@@ -6,6 +6,9 @@ | |||
#ifndef DLPACK_DLPACK_H_ | |||
#define DLPACK_DLPACK_H_ | |||
|
|||
/** | |||
* \brief Compatibility with C++ |
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.
This wasn't documented before which led to a warning when building documentation with Doxygen.
Thanks @tirthasheshpatel, this looks like a great start. @tqchen & others, allow me to give some context. We recently integrated DLPack support in NumPy and upgraded support in PyTorch. There were some loose ends there. And from all the discussions on this repo and on https://github.com/data-apis/array-api there is a lot to do in terms of docs, integration testing and possible new features & investigations/prototypes for DLPack itself. So some dedicated attention seemed warranted. Normally an internship would not be my first thought, since DLPack is quite a tricky project with a lot of stakeholders. However @tirthasheshpatel is already a SciPy maintainer and has done some excellent work there, so I am confident he can make some very nice contributions here. He will have almost 4 months to work on all of this, and on bugs/tests/upgrades in NumPy and other libraries. |
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.
nice work! it would be great to have some short instructions in README.md
or docs/README.md
on what commands to run to install the dependencies, build the docs, and serve them
Makefile
Outdated
$(MAKE) -C docs html | ||
|
||
show_docs: | ||
@python -c "import webbrowser; webbrowser.open_new_tab('file:https://$(PWD)/docs/build/html/index.html')" |
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.
its probably more robust to start an http server to better replicate the final gh-pages environment and avoid any CORS issues from popping up
@python -c "import webbrowser; webbrowser.open_new_tab('file:https://$(PWD)/docs/build/html/index.html')" | |
@python3 -m http.server --directory docs/build/html |
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.
Done, thanks for the suggestion!
docs/source/conf.py
Outdated
# Add any paths that contain custom static files (such as style sheets) here, | ||
# relative to this directory. They are copied after the builtin static files, | ||
# so a file named "default.css" will overwrite the builtin "default.css". | ||
html_static_path = ['_static'] |
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.
This folder doesn't exist in the repo, so commenting it out hides the sphinx warning
html_static_path = ['_static'] | |
# html_static_path = ['_static'] |
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.
Done.
.github/workflows/docs.yaml
Outdated
sudo apt-get -y install python3.9 build-essential doxygen | ||
curl https://bootstrap.pypa.io/get-pip.py -o get-pip.py | ||
python3.9 get-pip.py | ||
python3.9 -m pip install -U pip wheel | ||
python3.9 -m pip install cmake ninja |
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.
iirc GitHub Actions comes pre-configured with python 3.8 and pip, so you don't need to install it manually
sudo apt-get -y install python3.9 build-essential doxygen | |
curl https://bootstrap.pypa.io/get-pip.py -o get-pip.py | |
python3.9 get-pip.py | |
python3.9 -m pip install -U pip wheel | |
python3.9 -m pip install cmake ninja | |
sudo apt-get -y install build-essential doxygen | |
pip install ninja cmake wheel |
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.
Changed to use default Python 3.8.
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.
Hi @tirthasheshpatel! Great work on these docs!
I had a minor suggestion around the sphinx configs, I ran this locally and was able to reproduce an error-free run.
docs/source/conf.py
Outdated
# Add any paths that contain custom static files (such as style sheets) here, | ||
# relative to this directory. They are copied after the builtin static files, | ||
# so a file named "default.css" will overwrite the builtin "default.css". | ||
html_static_path = ['_static'] |
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.
If there are no static files in this documentation build, please remove the _static
path.
html_static_path = ['_static'] | |
html_static_path = [] |
See here: readthedocs/readthedocs.org#1776
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.
I just commented this line out as @driazati suggested. So that if the docs ever evolve to use static files and/or templates, we can just uncomment these lines.
.github/workflows/docs.yaml
Outdated
# directory if they don't exist already. This needs to be done for | ||
# Sphinx to run without errors. It tries to find these directories and | ||
# fails if they don't exist. | ||
mkdir -p ./docs/source/_static |
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.
I think you should be able to remove this line if you set html_static_path
in docs/source/conf.py
as suggested.
mkdir -p ./docs/source/_static |
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.
Done.
docs/source/conf.py
Outdated
extensions = ['breathe'] | ||
|
||
# Add any paths that contain templates here, relative to this directory. | ||
templates_path = ['_templates'] |
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.
If there are no templates in this documentation build, please remove the _templates
path.
templates_path = ['_templates'] | |
templates_path = [] |
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.
Commented the line out.
.github/workflows/docs.yaml
Outdated
# Sphinx to run without errors. It tries to find these directories and | ||
# fails if they don't exist. | ||
mkdir -p ./docs/source/_static | ||
mkdir -p ./docs/source/_templates |
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.
I think you should be able to remove this line if you set templates_path
in docs/source/conf.py
as suggested.
mkdir -p ./docs/source/_templates | |
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.
Done.
.github/workflows/docs.yaml
Outdated
# create the _static and _templates directories under the docs/source | ||
# directory if they don't exist already. This needs to be done for | ||
# Sphinx to run without errors. It tries to find these directories and | ||
# fails if they don't exist. |
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.
These are configurable options controlled via docs/source/conf.py
. You probably just need to configure this slightly differently, and then you won't have to create empty directories for _static
and _templates
to make this run without errors.
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.
Yes, I have changed the conf file according to the suggestions here. Let me know if this looks good to you now!
- Change `show_docs` target to use http.server instead of webbrowser - Comment out `html_static_path` and `html_templates_path` to avoid Sphinx Warnings - Use the default Python 3.8 in CI
Thanks for the reviews @driazati, @denise-k, @junrushao1994! I think I have addressed all the comments. Let me know if the changes look good! |
Will this be hosted via github pages? |
That's the plan to host docs initially. Right now, I haven't added anything to do that. I can add it here if the reviewers are happy with it. Otherwise, it can also be done in a follow-up PR. |
github pages seems the most reasonable for now |
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. Tested locally and seems to work as expected.
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, thanks for working on this!
Hi everyone,
I am Tirth, an intern at Quansight Labs. I will be working on DLPack under the mentorship of @mattip to add documentation, tests, and contributing bug-fixes/features to other packages that use DLPack (e.g. NumPy, CuPy) and DLPack itself.
This PR introduces Sphinx as a documentation tool for DLPack. It uses the Breathe extension to parse Doxygen output (because Sphinx doesn't directly parse C/C++). The new documentation uses the PyData Sphinx Theme (adopted by several projects in the data science community). To keep the diff minimal, I didn't change the default configuration but it can be changed later to add sidebars, logos, social media links, etc. This PR also adds a small CI script to build docs (just to test if Make and CMake changes work properly).
I have deployed the documentation here for testing. In a follow-up PR, we can add a script to deploy the docs on
gh-pages
branch of the main repo. Then we can start populating the docs with examples, tutorials, list of projects that use DLPack, etc.I would appreciate your feedback and thoughts on this PR, thanks!
cc @rgommers