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

Check for missing fovs after run is complete #410

Merged
merged 29 commits into from
Aug 31, 2023
Merged

Conversation

camisowers
Copy link
Contributor

@camisowers camisowers commented Aug 7, 2023

If you haven't already, please read through our contributing guidelines before opening your PR

What is the purpose of this PR?

Closes #399. Checks that bin/json files are generated for all FOVs outlined in the run file.

Also addresses the watcher edge case where a run crashes or does not output the bin/json files of one of the fovs in the run file. Currently the watcher notebook will hang forever and ever, since our current FOV timeout system is based on how long it takes for the full bin file to be completely generated (only triggered once it is written with size zero).

How did you implement your changes

Create a helper function to add to notebook 3f and also implement a run callback in watcher.

Add parallel function file_timer() that checks the time elapsed since the last FOV file was written. If the elapsed time is greater than 3x the normal FOV timeout period, the watcher function will call the run callbacks and then exit.

Remaining issues

  • add error check in watcher tests
  • add notebook implementations

@camisowers camisowers added the enhancement New feature or request label Aug 7, 2023
@camisowers camisowers self-assigned this Aug 7, 2023
@camisowers
Copy link
Contributor Author

camisowers commented Aug 8, 2023

@alex-l-kong Before I implement any more watcher testing, will you take a look? I just realized that with the watcher now checking for completion based on the FOVs provided in the json file, this new callback might not ever raise an error. Is there a scenario where the run callbacks will get called even when a FOV was skipped?

Copy link
Contributor

@alex-l-kong alex-l-kong left a comment

Choose a reason for hiding this comment

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

missing_fov_check should still error out properly since the watcher also marks FOVs that time out as completed, however, these FOVs won't have any extracted data. The function does need to account for one additional edge case.

src/toffy/json_utils.py Outdated Show resolved Hide resolved
src/toffy/json_utils.py Show resolved Hide resolved
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@camisowers camisowers marked this pull request as ready for review August 28, 2023 23:26
Copy link
Contributor

@srivarra srivarra left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just a clarification question.

src/toffy/fov_watcher.py Show resolved Hide resolved
Copy link
Contributor

@alex-l-kong alex-l-kong left a comment

Choose a reason for hiding this comment

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

Minor detail about the missing FOV timeout print statement.

src/toffy/fov_watcher.py Outdated Show resolved Hide resolved
Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Looks good

@ngreenwald ngreenwald merged commit 78b31bb into main Aug 31, 2023
4 checks passed
@ngreenwald ngreenwald deleted the missing_fov_check branch August 31, 2023 00:25
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.

Check for missing FOVs
4 participants