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 testing and runtime checks for additional stores #59

Merged
merged 3 commits into from
Feb 22, 2024

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Jan 30, 2024

This PR adds some integration tests that make use of additional_stores. In the process, I also added a check that makes sure that any additional_stores requested by a job are checked before the job executes, rather than failing afterwards.

We could also add a check at submission time for this, which should be a future PR.

Closes #76

@ml-evs ml-evs added the testing For issues/PRs relating to testing label Jan 30, 2024
@ml-evs ml-evs force-pushed the ml-evs/test-additional-stores branch 2 times, most recently from 7133f25 to e9d94c4 Compare February 17, 2024 14:28
@ml-evs ml-evs changed the title [WIP] Add test for additional stores with e.g., GridFS Add tests for additional stores Feb 18, 2024
@ml-evs ml-evs force-pushed the ml-evs/test-additional-stores branch from 4abf99b to ce2ced0 Compare February 18, 2024 16:41
@ml-evs ml-evs marked this pull request as ready for review February 18, 2024 16:41
@ml-evs ml-evs force-pushed the ml-evs/test-additional-stores branch from ce2ced0 to f953403 Compare February 18, 2024 16:47
@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2024

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (38900fd) 44.62% compared to head (92cb740) 45.19%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #59      +/-   ##
===========================================
+ Coverage    44.62%   45.19%   +0.57%     
===========================================
  Files           42       42              
  Lines         4267     4290      +23     
  Branches       869      876       +7     
===========================================
+ Hits          1904     1939      +35     
+ Misses        2184     2173      -11     
+ Partials       179      178       -1     
Files Coverage Δ
src/jobflow_remote/remote/data.py 83.51% <100.00%> (+14.76%) ⬆️
src/jobflow_remote/testing/__init__.py 47.05% <38.46%> (-7.49%) ⬇️

... and 3 files with indirect coverage changes

@ml-evs ml-evs force-pushed the ml-evs/test-additional-stores branch 2 times, most recently from bfd7d74 to e105ba2 Compare February 19, 2024 15:35
src/jobflow_remote/remote/data.py Outdated Show resolved Hide resolved
src/jobflow_remote/remote/data.py Outdated Show resolved Hide resolved
@ml-evs ml-evs force-pushed the ml-evs/test-additional-stores branch from f4c46d3 to 9073c8c Compare February 20, 2024 17:31
@ml-evs ml-evs changed the title Add tests for additional stores Add testing and runtime checks for additional stores Feb 22, 2024
@ml-evs ml-evs requested a review from gpetretto February 22, 2024 13:05
- Manually connect to gridfs and check for file

- Read from GridFS store directly

- Fix typo
- Use inspect instead of wrapped annotations, and return `RemoteError` when store is not present

- Tweak store tests

- Fix bad auto editor import

- Dial back the size of test data in the GridFS test
@ml-evs ml-evs force-pushed the ml-evs/test-additional-stores branch from 9073c8c to 92cb740 Compare February 22, 2024 13:06
@ml-evs ml-evs merged commit 5da2c9e into develop Feb 22, 2024
5 checks passed
@ml-evs ml-evs deleted the ml-evs/test-additional-stores branch March 26, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing For issues/PRs relating to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add guard for missing additional_store before executing job
3 participants