Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves documentation build reliability and makes several unit tests safe to run in parallel (e.g., under pytest -n 8) by isolating filesystem output and environment variables per test.
Changes:
- Refactor multiple tests to use per-test (or per-class) temporary directories and restore environment variables in teardown.
- Fix/cleanup minor doc-build issues (Sphinx Makefile default, additional mocked imports, and an invalid-escape warning in a Matplotlib label).
- Expand CI docs job to also run
make htmlin addition to a directsphinx-buildinvocation.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| py/desisim/test/test_quickgen.py | Move test I/O + env setup to per-test temp dirs for parallel safety. |
| py/desisim/test/test_quickcat.py | Write generated FITS files into an isolated temp directory and cleanup via TemporaryDirectory. |
| py/desisim/test/test_pixsim.py | Isolate env + output paths per test using TemporaryDirectory. |
| py/desisim/test/test_obs.py | Isolate env + output paths per test using TemporaryDirectory. |
| py/desisim/test/test_io.py | Create per-test temp dir + unique output filename; restore env in teardown. |
| py/desisim/test/test_batch.py | Generate batch script in a per-test temp dir; restore env reliably. |
| py/desisim/spec_qa/redshifts.py | Use a raw string for a Matplotlib label to avoid invalid escape sequence warnings. |
| py/desisim/qso_template/desi_qso_templ.py | Remove unused imp import. |
| doc/conf.py | Mock additional desitarget submodules during autodoc to avoid doc-build import failures. |
| doc/changes.rst | Update changelog entries/links for unreleased version notes. |
| doc/Makefile | Default SPHINXBUILD to sphinx-build (more portable default). |
| .github/workflows/python-package.yml | Add CI step to verify make html doc build path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
weaverba137
left a comment
There was a problem hiding this comment.
I had a few questions that may lead to minor changes, but overall looks good.
|
One more question: do we know how many cores are available when running GitHub Actions? |
I have not found any specific mention of multiple cores available to GitHub actions; their primary parallelism appears to be splitting the various jobs to e.g. test different versions of python in parallel. Regardless, adding test parallelism safety makes testing locally much much faster even if GitHub remains single-core per |
weaverba137
left a comment
There was a problem hiding this comment.
I think this is ready to go. Thanks for the changes.
This PR started out as a minor cleanup for building docs in prep for a new tag, but morphed into also adding parallelism safety for tests (run with pytest-xdist via
pytest -n 8).