search

probml / pyprobml

Python code for "Probabilistic Machine learning" book by Kevin Murphy
MIT License
6.39k stars 1.51k forks source link

Make PR code-tests more user friendly #934

Open gerdm opened 2 years ago

gerdm commented 2 years ago

Making a PR with appropriate code seems to be independent of the result for the code checks. This makes it hard to test if the PR checks are failing because of my code or something else.

For instance, in #933, I created a new notebook and did not modify any other notebook. There are 24 tests, I passed 4 and failed 20. Looking through some of the unit tests that failed, I see that 183 notebooks were tested and one failed. I got the error:

Oh no! 💥 💔 💥
1 file would be reformatted, 183 files would be left unchanged.
Error: Process completed with exit code 1.

But I didn't reformat any existing notebook

All of the failures seem to be completely outside the scope of my PR.

=================================== FAILURES ===================================
[107](https://github.com/probml/pyprobml/runs/7059536084?check_suite_focus=true#step:7:108)
__________ test_run_notebooks[notebooks/book2/29/ggm_fit_demo.ipynb] ___________
[108](https://github.com/probml/pyprobml/runs/7059536084?check_suite_focus=true#step:7:109)
[gw1] linux -- Python 3.7.13 /github/home/miniconda3/envs/py37/bin/python
[109](https://github.com/probml/pyprobml/runs/7059536084?check_suite_focus=true#step:7:110)

Suggestions:

  1. Unit-test only the modified / changed notebook
  2. Create an .md file describing what each unit-test is doing so that people who make PRs understand what fails.
patel-zeel commented 2 years ago

@gerdm Yes, you are right about confusing workflow for the contributors. The first check checks if notebooks are black formatted or not. It seems that your notebook needs black formatting. Also, have you installed pre-commit locally? because ideally, it should have prevented the code_quality check from failing.

patel-zeel commented 2 years ago

@gerdm I have made some quick changes in name of the jobs to make it a bit easier. I have also implemented your suggestion of testing only the modified/changed notebooks. Can you pull the master in #933 and see if it works?