Add Sphinx to build documentation for DLPack #90

tirthasheshpatel · 2022-01-27T12:13:55Z

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

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).

tirthasheshpatel

Some comments to help reviews get started.

tirthasheshpatel · 2022-01-27T12:23:12Z

doc_requirements.txt

@@ -0,0 +1,3 @@
+Sphinx==4.4.0
+pydata-sphinx-theme==0.7.1


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.

tirthasheshpatel · 2022-01-27T12:24:50Z

include/dlpack/dlpack.h

@@ -6,6 +6,9 @@
 #ifndef DLPACK_DLPACK_H_
 #define DLPACK_DLPACK_H_

+/**
+ * \brief Compatibility with C++


This wasn't documented before which led to a warning when building documentation with Doxygen.

include/dlpack/dlpack.h

rgommers · 2022-01-27T17:54:47Z

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.

driazati

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

driazati · 2022-01-27T20:32:09Z

Makefile

+	$(MAKE) -C docs html
+
+show_docs:
+	@python -c "import webbrowser; webbrowser.open_new_tab('file://$(PWD)/docs/build/html/index.html')"


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

Suggested change

@python -c "import webbrowser; webbrowser.open_new_tab('file://$(PWD)/docs/build/html/index.html')"

@python3 -m http.server --directory docs/build/html

Done, thanks for the suggestion!

driazati · 2022-01-27T20:33:46Z

docs/source/conf.py

+# 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']


This folder doesn't exist in the repo, so commenting it out hides the sphinx warning

Suggested change

html_static_path = ['_static']

# html_static_path = ['_static']

driazati · 2022-01-27T20:42:07Z

.github/workflows/docs.yaml

+        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


iirc GitHub Actions comes pre-configured with python 3.8 and pip, so you don't need to install it manually

Suggested change

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

Changed to use default Python 3.8.

denise-k

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.

denise-k · 2022-01-27T20:42:11Z

docs/source/conf.py

+# 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']


If there are no static files in this documentation build, please remove the _static path.

Suggested change

html_static_path = ['_static']

html_static_path = []

See here: readthedocs/readthedocs.org#1776

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.

denise-k · 2022-01-27T20:43:39Z

.github/workflows/docs.yaml

+        # 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


I think you should be able to remove this line if you set html_static_path in docs/source/conf.py as suggested.

Suggested change

mkdir -p ./docs/source/_static

denise-k · 2022-01-27T20:49:36Z

docs/source/conf.py

+extensions = ['breathe']
+
+# Add any paths that contain templates here, relative to this directory.
+templates_path = ['_templates']


If there are no templates in this documentation build, please remove the _templates path.

Suggested change

templates_path = ['_templates']

templates_path = []

Commented the line out.

denise-k · 2022-01-27T20:51:29Z

.github/workflows/docs.yaml

+        # 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


I think you should be able to remove this line if you set templates_path in docs/source/conf.py as suggested.

Suggested change

mkdir -p ./docs/source/_templates

denise-k · 2022-01-27T20:52:54Z

.github/workflows/docs.yaml

+        # 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.


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.

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

tirthasheshpatel · 2022-01-28T09:46:17Z

Thanks for the reviews @driazati, @denise-k, @junrushao1994! I think I have addressed all the comments. Let me know if the changes look good!

mattip · 2022-01-28T09:47:47Z

Will this be hosted via github pages?

tirthasheshpatel · 2022-01-28T09:51:06Z

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.

edgarriba · 2022-01-28T10:14:08Z

Will this be hosted via github pages?

github pages seems the most reasonable for now

edgarriba

LGTM. Tested locally and seems to work as expected.

denise-k

LGTM, thanks for working on this!

tqchen · 2022-02-01T13:53:39Z

Thanks @tirthasheshpatel @denise-k @driazati @junrushao1994 @edgarriba !

tirthasheshpatel added 3 commits January 27, 2022 14:37

Pin versions

fdb0167

Use a previous stable version of pydata sphinx theme

2e1ce69

tirthasheshpatel commented Jan 27, 2022

View reviewed changes

driazati reviewed Jan 27, 2022

View reviewed changes

denise-k suggested changes Jan 27, 2022

View reviewed changes

Address code reviews

41f7e5e

- 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

edgarriba approved these changes Jan 28, 2022

View reviewed changes

tirthasheshpatel added 3 commits January 28, 2022 16:33

Add instructions in docs/README.md to build documentation

cec455e

Fix a typo in the docs/README.md file

f09018a

doc_requirement.txt -> doc_requirements.txt in docs/README.md

8eb4fd3

denise-k approved these changes Jan 28, 2022

View reviewed changes

tqchen approved these changes Jan 31, 2022

View reviewed changes

driazati approved these changes Jan 31, 2022

View reviewed changes

tqchen merged commit 7a888dc into dmlc:main Feb 1, 2022

tirthasheshpatel deleted the docs-sphinx branch February 2, 2022 11:08

tirthasheshpatel mentioned this pull request Feb 2, 2022

Add a script to deploy docs on gh-pages branch #92

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Sphinx to build documentation for DLPack #90

Add Sphinx to build documentation for DLPack #90

tirthasheshpatel commented Jan 27, 2022

tirthasheshpatel left a comment

tirthasheshpatel Jan 27, 2022 •

edited

tirthasheshpatel Jan 27, 2022

rgommers commented Jan 27, 2022 •

edited

driazati left a comment

driazati Jan 27, 2022

tirthasheshpatel Jan 28, 2022

driazati Jan 27, 2022 •

edited

tirthasheshpatel Jan 28, 2022

driazati Jan 27, 2022

tirthasheshpatel Jan 28, 2022

denise-k left a comment

denise-k Jan 27, 2022

tirthasheshpatel Jan 28, 2022 •

edited

denise-k Jan 27, 2022

tirthasheshpatel Jan 28, 2022

denise-k Jan 27, 2022

tirthasheshpatel Jan 28, 2022

denise-k Jan 27, 2022

tirthasheshpatel Jan 28, 2022

denise-k Jan 27, 2022

tirthasheshpatel Jan 28, 2022

tirthasheshpatel commented Jan 28, 2022

mattip commented Jan 28, 2022

tirthasheshpatel commented Jan 28, 2022

edgarriba commented Jan 28, 2022

edgarriba left a comment

denise-k left a comment

tqchen commented Feb 1, 2022

	@python -c "import webbrowser; webbrowser.open_new_tab('file://$(PWD)/docs/build/html/index.html')"
	@python3 -m http.server --directory docs/build/html

	html_static_path = ['_static']
	# html_static_path = ['_static']

		@@ -0,0 +1,3 @@
		Sphinx==4.4.0
		pydata-sphinx-theme==0.7.1

Add Sphinx to build documentation for DLPack #90

Add Sphinx to build documentation for DLPack #90

Conversation

tirthasheshpatel commented Jan 27, 2022

tirthasheshpatel left a comment

Choose a reason for hiding this comment

tirthasheshpatel Jan 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgommers commented Jan 27, 2022 • edited

driazati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

driazati Jan 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denise-k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tirthasheshpatel Jan 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tirthasheshpatel commented Jan 28, 2022

mattip commented Jan 28, 2022

tirthasheshpatel commented Jan 28, 2022

edgarriba commented Jan 28, 2022

edgarriba left a comment

Choose a reason for hiding this comment

denise-k left a comment

Choose a reason for hiding this comment

tqchen commented Feb 1, 2022

tirthasheshpatel Jan 27, 2022 •

edited

rgommers commented Jan 27, 2022 •

edited

driazati Jan 27, 2022 •

edited

tirthasheshpatel Jan 28, 2022 •

edited