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

Tox integration & use in GitHub actions #107

Merged
merged 49 commits into from
Jun 25, 2024
Merged

Tox integration & use in GitHub actions #107

merged 49 commits into from
Jun 25, 2024

Conversation

jared321
Copy link
Contributor

@jared321 jared321 commented Apr 10, 2024

The work associated with Issue #106 appears to be finished. The following are the steps I will carry out as part of a self-review of this PR

  • Full review of all changes made to the code
  • Run all tox tasks locally and check products
  • Add bad style to file and see tox and CI action fail as expected
  • Break code and see nocoverage/coverage fail in tox and in all CI actions as expected
  • Breaks docs so that a warning is issued. Confirm warning treated as error with tox and CI actions as expected.
  • Breaks docs so that an error is issued. Confirm error with tox and CI actions as expected.
  • Alter code so that coverage changes. Check for changes in local tox report and Coveralls.
  • Download HTML and PDF docs and sanity check contents. Review tox section carefully.
  • Download coverage reports and sanity check
  • Read through all action logs for correctness

I injected many different errors into the sphinx documentation including using tx.rst instead of tox.rst when including the latter and writing :mah: instead of :math:. In such cases, sphinx-build reports these as errors in its output but returns a zero exit code, which indicates success. I saw this when running sphinx-build from the command line and tox reports success. When I use the flag --fail-on-warning with sphinx-build, the output states that these are warnings that are being treated as errors and a nonzero exit code is emitted. It appears that sphinx-build does not have errors or handle these in normal ways. Therefore, I would never use sphinx-build in automated processes without that flag.

Note that this work has also instituted the necessary changes so that Python 3.8 is no longer a supported version. See Issue #112. All tests are passing with 3.12 for Windows, Ubuntu, and macOS. I updated all actions that use a single Python version to use 3.12 except for building sphinx document, which reported an error associated with loading sphinxcontrib.bibtex.

I have changed the RTD setup so that build warnings are treated as errors. We need to check that manually once this branch is in main.

@mosesyhc I have finished my review and have added you as a reviewer. Some potentially useful information for you:

  • When I run the test suite locally, I get three warnings about transposing f. However, some CI testing actions report (e.g. macOS 3.11) the following as well
tests/test_emu_cal/test_cal_mh.py::test_cal_MLcal[input13-input23-expectation3]
  /home/runner/work/surmise/surmise/surmise/utilitiesmethods/metropolis_hastings.py:80: RuntimeWarning: overflow encountered in exp
    p_accept = min(1, np.exp(logpost - lposterior[i-1]))
  • Also, the Windows testing actions logs do not suggest that the test suite was actually run. In particular, there is no standard output logging from your testing script.

Was able to run all tox tasks successfully.  Did quick review of coverage
reports and sphinx output.  All looked acceptable.  Flake8 shows all passing,
which is as to be expected since it is already integrated in the repo.  Sphinx
output printed a few warnings.
Built the HTML form of the docs and all looks good.  The language should be
cleaned up during PR review.
@jared321 jared321 added the enhancement New feature or request label Apr 10, 2024
jared321 and others added 17 commits April 10, 2024 17:20
The 3.8/macOS python-package job failed due to a scipy package with a
wrong/suspicious hash.  It was trying to install 1.4, which is less than the
minimum value of 1.7 specified for the package.  Trying 1.7 to see if it passes
again.  Sphinx package builds failed with a strange error.  I find in stack
overflow that this error message can be related with shallow git clones.
Asking action to download the full repo as suggested to see if that fixes it.
Trying to see if updating to latest version of GH action tools works.
Currently HTML and PDF forms of Sphinx docs build without errors and warnings.
To keep the docs in the high-level of cleanliness, we ask RTD to treat warnings
as errors.  I'm presently not using this setting with tox so that users get the
normal experience there.
@jared321 jared321 self-assigned this Jun 24, 2024
@jared321
Copy link
Contributor Author

@mosesyhc The repo currently specifies dependencies in pyproject.toml and requirements.txt. Can we get rid of the latter?

@mosesyhc
Copy link
Member

@mosesyhc The repo currently specifies dependencies in pyproject.toml and requirements.txt. Can we get rid of the latter?

Yes, we can.

@mosesyhc
Copy link
Member

@jared321 : I removed the dependencies mentions in README.

The building of docs is inherited from the first version of surmise. Since we have effectively included the capability in tox, we can simply point to the tox info.

jared321 added 21 commits June 24, 2024 16:21
Flake8 should complain about importing but not using os and not having enough
blank lines.  THIS SHOULD BE REVERTED!
A single test should fail because it incorrectly expects TypeError instead of
ValueError.  THIS SHOULD BE REVERTED!
We should see the warning treated as an error by CI.  THIS SHOULD BE REVERTED!
I had a very difficult time trying to inject a true error.  I was able to get
the sphinx-build output to say that it found "errors" by writing "tx.rst"
instead of "tox.rst" when including that file and by writing ":mth:" instead of
":math:".  However, neither of these actually resulted in a non-zero exit code
unless I used the treat warnings as errors flag.  In this case, the
sphinx-build output says that it is treating a warning as an error.  Therefore,
I would *never* build with sphinx without this flag in an automated scheme
since it appears that sphinx doesn't actually have errors.
@jared321 jared321 requested a review from mosesyhc June 25, 2024 15:07
Copy link
Member

@mosesyhc mosesyhc left a comment

Choose a reason for hiding this comment

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

I reviewed by performing these tasks:

  • I have installed tox locally following tox instruction.
  • I ran the tox commands and verified the usages.
    • tox -e check is scanning every directory under surmise, including local builds and IDE-generated venvs. I am including these into .flake8 ignores.

All look fine to me otherwise.

@jared321 jared321 changed the title DRAFT: Tox integration & use in GitHub actions Tox integration & use in GitHub actions Jun 25, 2024
@jared321 jared321 merged commit 340edd8 into develop Jun 25, 2024
16 checks passed
@mosesyhc mosesyhc deleted the 106_tox branch August 2, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants