Skip to content

Conversation

@mathiasburger
Copy link

RFC for using sphinx doctest and continously migrating the doctest code to being tested

Fixes #848

Changes

  • Updated docs/Makefile with a doctest target
  • Enabled the sphinx extension sphinx.ext.doctest in docs/source/conf.py
  • A minimal example of an updated docstring in torchdata/dataloader2/adapter.py
  • Adding the doctest step to the CI in .github/workflows/_build_test_upload.yml

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 21, 2022
@mathiasburger mathiasburger force-pushed the feature/sphinx_doctest branch 2 times, most recently from c2558d2 to 4b4cbdf Compare October 21, 2022 15:24
run: |
cd ./docs
pip3 install -r requirements.txt
make doctest
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather put this test into ci.yml. This _build_test_upload is the one we upload compiled binaries.

All PR will execute GHA in ci.yml.

Copy link
Author

Choose a reason for hiding this comment

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

I have put it into ci.yml now.

@mathiasburger mathiasburger force-pushed the feature/sphinx_doctest branch from 4b4cbdf to caadff9 Compare October 24, 2022 11:49
Copy link
Contributor

@ejguan ejguan left a comment

Choose a reason for hiding this comment

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

I think this PR looks good. Thank you! Since we have enabled the ci for doctest, we might need to fix all docstring in this PR.

@mathiasburger mathiasburger force-pushed the feature/sphinx_doctest branch from caadff9 to dd7c980 Compare October 27, 2022 11:18
@mathiasburger
Copy link
Author

I think this PR looks good. Thank you! Since we have enabled the ci for doctest, we might need to fix all docstring in this PR.

If you say that this kind of setup is the way to move forward, I am happy to committing to do further migration of the docstrings. Maybe we can go package by package to keep the PRs manageable?

The approach allows continuous migration and as a finalization we could then remove doctest_test_doctest_blocks config setting in the final PR to make sure that no untested doctest blocks starting with >>> ... can get into new code.

@ejguan
Copy link
Contributor

ejguan commented Oct 27, 2022

Maybe we can go package by package to keep the PRs manageable?

Agree. Thank you!

@facebook-github-bot
Copy link
Contributor

@ejguan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@ejguan ejguan left a comment

Choose a reason for hiding this comment

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

Thank you for this effort. LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RFC] Verify that the docs contain working code and self-contained examples using doctest

4 participants