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 timeout option #56

Closed
joeHickson opened this issue Aug 25, 2020 · 6 comments · Fixed by #63
Closed

Add timeout option #56

joeHickson opened this issue Aug 25, 2020 · 6 comments · Fixed by #63
Assignees
Labels
enhancement New feature or request

Comments

@joeHickson
Copy link
Collaborator

Provide a (crude) mechanism for ending individual location processing after a timeout.
https://www.rdocumentation.org/packages/R.utils/versions/2.9.2/topics/withTimeout

Note (re: stan code keeping running):
"More precisely, if a function is implemented in native code (e.g. C) and the developer of that function does not check for user interrupts, then you cannot interrupt that function neither via a user interrupt (e.g. Ctrl-C) nor via the built-in time out mechanism. To change this, you need to contact the developer of that piece of code and ask them to check for R user interrupts in their native code."
I have found reference to Rstan checking for interrupts every iteration so we'll have to see what happens.

@joeHickson
Copy link
Collaborator Author

To give visibility of processes being killed this needs a mechanism to return the identity of any failures

@seabbs seabbs added the enhancement New feature or request label Aug 25, 2020
@joeHickson joeHickson self-assigned this Aug 26, 2020
@joeHickson
Copy link
Collaborator Author

good news! Rstan does indeed seem to be killed and die.
bad news - funnily enough the summary processing doesn't handle missing data very well!

@seabbs do you have any feeling about how we should represent No Data Available / timeout in the estimate summary? I think the regional epinow route is using the out object to hold the data so it doesn't re-load from disk, and it also has the full list of locations in the region.

@seabbs
Copy link
Contributor

seabbs commented Aug 26, 2020

That is great news and yes I can imagine.

So at the moment, the expectation is that when read in from the out object subregions that have failed will have NULL. These then get thrown away in regional_summary and everything continues as normal. When using regional_summary and reading in from disk I think it may be less fault-tolerant (with this bit of code:

estimates <- purrr::map(regions, ~ load_data(file = "summary.rds", region = .))
potentially being an issue but this bit:

EpiNow2/R/get.R

Line 113 in fafb618

load_data <- purrr::safely(EpiNow2::get_raw_result)
being fault-tolerant). Similarly, a NULL in the output indicates any failure.

Could continue this for subregions that have been timed out with an informative log entry or add a message to the out object/into the results folder but this would then add a bit of a headache for results processing.

When saving it would make sense to save a killed due to time-out flag definitely.

When outputting results (we stick with this duality, by the way, to stay being CRAN compliant + acting like a normal package whilst also supporting saving to folders in order to make producing regional results more independent) we could add another list entry on the top level to the out object. At the moment it has $regional and $summary. Could also add $timeout and list subregions that have been timed out (could do the same thing for regions that have errored). If going with this approach I guess it would make sense to save this information into the summary folder (though that adds complexity in passing objects into regional_summary whilst trying to maintain interoperability between reading in from disk and passing in objects).

Sorry - a bit of ramble - happy to clarify any of the above that is unclear.

@joeHickson
Copy link
Collaborator Author

I have only put the timeout in the regional_epinow function which always returns a result set from the epinow call - this might mean we can sidestep the re-loading from disk issue.

@joeHickson
Copy link
Collaborator Author

@seabbs Please could you add @JAllen42 so he can put a bit of work in on this? Thanks

@seabbs
Copy link
Contributor

seabbs commented Aug 27, 2020

Sorry yes, I missed that before. Done

@joeHickson joeHickson mentioned this issue Sep 1, 2020
1 task
@seabbs seabbs closed this as completed in #63 Sep 3, 2020
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 a pull request may close this issue.

2 participants