search

LLNL / MuyGPyS

A fast, pure python implementation of the MuyGPs Gaussian process realization and training algorithm.
Other
23 stars 11 forks source link

Tests failing is not blocking #132

Closed igoumiri closed 1 year ago

igoumiri commented 1 year ago

This typo https://github.com/LLNL/MuyGPyS/blob/a849e40177aaf30e4bfd7c6dbaded8f3db8ff887/.github/workflows/develop-test.yml#L42

makes me think that the tests can fail undetected. They might need to be chained with &&.

igoumiri commented 1 year ago

Relatedly, I ran this https://github.com/LLNL/MuyGPyS/blob/a849e40177aaf30e4bfd7c6dbaded8f3db8ff887/.github/workflows/develop-test.yml#L33-L34

manually and it found a lot of issues. Is --exit-zero actually making it pass when it shouldn't?

alecmdunton commented 1 year ago

That's a good catch Imene. The second comment you added suggests that if there is a failure it will be reported as a warning, right?

igoumiri commented 1 year ago

I think so but you would only see the warnings if you looked for them.

bwpriest commented 1 year ago

Is --exit-zero actually making it pass when it shouldn't?

Actually, yes. flake8 is checking for syntax issues in the library. I had to add --exit-zero back when the config object worked differently, because we had to do some things that PEP guides do not endorse. We can turn the crashes back on now that the that process has been streamlined, although we'll have to ignore some error codes like F401 because of how MuyGPyS._src works. Removing --exit-zero will cause more CI crashes, since it will be a stickler for PEP-correct code, but it will also force us to maintain higher-quality code, so it might be worthwhile. Would y'all like me to turn --exit-zero off?

bwpriest commented 1 year ago

This typo

You're right, that is a mistake. I'll change it.

They might need to be chained with &&.

I don't think that is necessary.

bwpriest commented 1 year ago

PR #133 fixes the bug and updates the formatting throughout to match PEP requirements (where possible), and updates the flake8 linting call so that it now finds no issues. There are 2 items blocking me from merging this in:

  1. Current PRs should get merged first, as I had to change most of the files in the repo and it will be easier for me to merge those changes myself than to go the other way
  2. We should decide whether we actually want to turn off --exit-zero, since if we do small formatting foibles (e.g. unused imports) will cause the CI to crash until they are fixed. I am not opposed prima facie, but this change will certainly result in more failed CI checks for developers due to small issues. @alecmdunton and @igoumiri, please let me know what behavior you prefer.
igoumiri commented 1 year ago

I'm fine either way but I don't mind more failed CI checks. I rarely get things right on the first try anyway. 🙂

Also, I opened the issue because I thought failing tests were undetected. If that's not the case, we can close it.

alecmdunton commented 1 year ago

Let's turn off --exit-zero and clean up the library. It will be a pain at first but in the long run good.

bwpriest commented 1 year ago

Ok sounds good. Everyone might want to start practicing running flake8 before pushing to PRs. I'll wait to merge into develop until after the anisotropic PR gets merged.

bwpriest commented 1 year ago

I addressed this with PR #133. It is prudent to run sh scripts/flake/fatal.sh and sh scripts/flake/issues.sh before pushing commits from now on.