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

Add notebook smoke tests, QOL changes, part 2 #39

Merged
merged 8 commits into from
Oct 2, 2023

Conversation

matthewcarbone
Copy link
Collaborator

@matthewcarbone matthewcarbone commented Sep 14, 2023

Same as #38 but I actually remembered to pull from upstream first:

Just playing around/exploring the code a bit. While I was I figured I'd make a few QOL changes to the testing system:

  • Trying something new by adding a smoke test to the notebook. Figured it could be useful considering the notebooks are hosted on Colab and should always work. It's a fun hack that you can use: ipython -c "%run simpleGP.ipynb", for example.
  • Added osx-latest to the runs-on in the testing jobs. Since the project is open source your CI hours are free so it doesn't matter that it's a more expensive OS to test on.
  • Added python version 3.11 to the list of tested versions.
  • Added .flake8 and pyproject.toml files for running black (with line length 127).
  • Ran black on gpax.models.gp as a test (feel free to tell me you don't like this and I'll revert, but stylistically I think it's much easier to read!).

(there will no doubt be bugs which I will fix 👍)

@matthewcarbone
Copy link
Collaborator Author

@ziatdinovmax looks like these jobs just failed with no error messages... Doesn't even look like they ran. 😁
Any idea what's going on?

@matthewcarbone
Copy link
Collaborator Author

@ziatdinovmax Pretty sure I found the bug. I just did something really silly in the workflow files.

@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2023

Codecov Report

Merging #39 (b5348d0) into main (6d5bc4c) will decrease coverage by 0.04%.
The diff coverage is 95.23%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main      #39      +/-   ##
==========================================
- Coverage   95.60%   95.57%   -0.04%     
==========================================
  Files          39       39              
  Lines        3072     3072              
==========================================
- Hits         2937     2936       -1     
- Misses        135      136       +1     
Flag Coverage Δ
unittests 95.57% <95.23%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
gpax/models/gp.py 93.08% <95.23%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ziatdinovmax
Copy link
Owner

Looks like the notebook tests are failing.
@matthewcarbone can you please look into it?

@matthewcarbone
Copy link
Collaborator Author

@ziatdinovmax Yeah I think it's because I missed two notebook-specific dependencies. That's fixed now. Question is whether or not this will work anyway.

I do think notebooks should have smoke tests, but if this doesn't work I'll revert the smoke tests pieces and experiment elsewhere 😁. Once I get it right I can just open another PR.

@ziatdinovmax
Copy link
Owner

@matthewcarbone For some reason, I can't run the osx workflows. Given that JAX support on the Apple M series is still limited and buggy, I'm not sure how helpful they are anyway. Maybe you can remove them for now?

@matthewcarbone
Copy link
Collaborator Author

@ziatdinovmax yeah sure I can remove them. OSX is more expensive than Linux resource-wise, though it's odd it's been 13 hours...

Will remove them but definitely consider putting them back in the future. Note I don't believe that OSX latest == M1. Stay tuned.

@matthewcarbone
Copy link
Collaborator Author

@ziatdinovmax start of the new FY so I'll probably start working on this a bit more! How strict are you regarding the code coverage differences? I didn't actually change any code, I just put some new lines in for clarity 😁

@ziatdinovmax
Copy link
Owner

I wouldn't worry about the slight decrease in the code coverage due to basic formatting.

@ziatdinovmax ziatdinovmax merged commit 1e3f33a into ziatdinovmax:main Oct 2, 2023
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants