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

Run doctest and fix old examples #865

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Jun 27, 2024

Description

Spinoff from #858

Related Issue

  • Closes #
  • Related to #

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

@ricardoV94 ricardoV94 force-pushed the run_docest_module branch 4 times, most recently from e092f52 to 28c3f72 Compare June 27, 2024 19:32
@ricardoV94 ricardoV94 force-pushed the run_docest_module branch 3 times, most recently from c0cdf8e to f7e3a38 Compare June 27, 2024 20:06
@ricardoV94 ricardoV94 changed the title Run docest module Run docest and fix old examples Jun 27, 2024
@ricardoV94 ricardoV94 changed the title Run docest and fix old examples Run doctest and fix old examples Jun 27, 2024
@ricardoV94 ricardoV94 marked this pull request as ready for review June 27, 2024 20:11
if [[ $INSTALL_NUMBA == "1" ]]; then micromamba install --yes -q -c conda-forge "python~=${PYTHON_VERSION}=*_cpython" "numba>=0.57"; fi
if [[ $INSTALL_JAX == "1" ]]; then micromamba install --yes -q -c conda-forge "python~=${PYTHON_VERSION}=*_cpython" jax jaxlib numpyro && pip install tensorflow-probability; fi
if [[ $INSTALL_TORCH == "1" ]]; then micromamba install --yes -q -c conda-forge "python~=${PYTHON_VERSION}=*_cpython" pytorch pytorch-cuda=12.1 -c pytorch -c nvidia; fi

pip install pytest-sphinx
Copy link
Member Author

Choose a reason for hiding this comment

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

Only available via pip

@ricardoV94
Copy link
Member Author

ricardoV94 commented Jun 28, 2024

Doctests are working on the remote CI!

I couldn't find a way to trigger doctest locally automatically by playing with pyproject.toml so I didn't bother changing it.

I tried several flavors of:

[tool.pytest]
addopts = [
    "--durations=50",
    "--doctest-modules pytensor --ignore=pytensor/misc/check_duplicate_key.py --ignore=pytensor/link",
]
testpaths = [
    "pytensor",  # for doctests
    "tests/",
]

And then tested by seeing if this would return anything: pytest --collect-only | grep DoctestModule

So I've been using the explicit command (same that is in the CI) to run locally, which works

Copy link

codecov bot commented Jun 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.13%. Comparing base (7a00b88) to head (1634a2f).
Report is 164 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #865      +/-   ##
==========================================
+ Coverage   80.99%   81.13%   +0.14%     
==========================================
  Files         169      169              
  Lines       46939    46961      +22     
  Branches    11467    11478      +11     
==========================================
+ Hits        38019    38103      +84     
+ Misses       6713     6642      -71     
- Partials     2207     2216       +9     
Files with missing lines Coverage Δ
pytensor/gradient.py 77.37% <ø> (ø)
pytensor/graph/basic.py 88.61% <ø> (ø)
pytensor/misc/pkl_utils.py 79.48% <ø> (ø)
pytensor/scalar/basic.py 80.33% <ø> (ø)
pytensor/sparse/basic.py 82.53% <ø> (ø)
pytensor/tensor/basic.py 88.59% <ø> (+0.12%) ⬆️
pytensor/tensor/extra_ops.py 88.60% <ø> (ø)
pytensor/tensor/math.py 90.43% <ø> (ø)
pytensor/tensor/random/utils.py 100.00% <ø> (ø)
pytensor/tensor/rewriting/math.py 89.42% <ø> (ø)
... and 4 more

... and 7 files with indirect coverage changes

@ricardoV94
Copy link
Member Author

Ready for review

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

I checked the pytest-sphinx readme and initially I wasn't able to see any extra feature with respect to pytest. It does seem that with either of them using a specific path for test files and --doctest-modules at the same time does not work.

I have tried locally to run

pytest --doctest-modules --ignore=pytensor/misc/check_duplicate_key.py pytensor --ignore=pytensor/link  --benchmark-skip

and got 30 tests to run. The found 31 because pytensor/sparse/basic.py::pytensor.sparse.basic.block_diag uses the testcode directive, which means pytest-sphinx is needed to parse that as a test. Everywhere else direct doctest syntax is used so pytest alone recognizes the other 30 tests without pytest-sphinx.

It might be better to modify the test in sparse/basic to use doctest syntax and not add the extra dependency

@ricardoV94
Copy link
Member Author

In #858 I used pytest-sphinx as well, I like it more than the doctest approach? Is there a strong preference for not adding it as an extra-dependency on your side @OriolAbril?

@OriolAbril
Copy link
Member

No, just trying to keep things simple given doctest directive is nearly equivalent to testcode+testoutput, the only difference being block vs line execution, but if using directives to separate test code and output helps then use the extension

@ricardoV94 ricardoV94 merged commit f4e249d into pymc-devs:main Jul 8, 2024
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants