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 later_recurring #133

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

r2evans
Copy link

@r2evans r2evans commented Jul 6, 2020

Import the functionality of pool::scheduleTaskRecurring directly into later, with additional limits.

This PR is a combination of things I like to do with polling background processes. I'm familiar (a little) with coro, I think this addition fits better here in later than there.

Impetus: I have a bulk-query function that spawns a new process (processx calling sqlcmd) that takes several minutes (data volume, not unexpected). As an optional courtesy in my bulk-query function, I poll the process state and, when exited, I display a message on the console (prompting the user to do something with the new data).

So I do something akin to:

proc <- processx::process$new(...)
notify <- function() message("bulk query complete")
later_recurring(function() {
  alive <- proc$is_alive()
  if (!alive) notify()
  alive # returning logical FALSE results in self-cancellation
}, delay = 10, limit = 100)

Having a never-ending recurring loop might be problematic (and concerning), so there are two additions to pool::scheduleTaskRecurring:

  • self-cancelling func: if the udf returns FALSE, then it is self-cancelling; if nothing (or not the literal FALSE) is returned, it will not self-cancel;
  • limit=NA (the default) allows limitless execution, setting limit=9 permits only 9 iterations before self-cancelling.

Bill Evans and others added 3 commits July 6, 2020 08:55
- if the 'func()' returns a logical, then its return value is taken as
  a "continue" logical; that is, if 'func()' returns true, then
  continue, if false then stop (the recurrency is cancelled)
@r2evans
Copy link
Author

r2evans commented Nov 30, 2021

I believe the routine failure is due to GH actions and nothing to do with the patch itself. That is, I'm not sure why the action is trying to push into my fork for this PR:

Collecting information about PR #133...
git push ***github.com/r2evans/later.git -q HEAD:add/later_recurring
remote: Permission to r2evans/later.git denied to github-actions[bot].
fatal: unable to access 'https://github.com/r2evans/later.git/': The requested URL returned error: 403
Error: The process 'git' failed with exit code 128

@wch, is there something I need to do to the PR to fix that failure?

@schloerke
Copy link
Collaborator

@r2evans There is some documentation updates to be made. But the actions should be auto pushing it back.
I'm working on a fix for the push issue

@r2evans
Copy link
Author

r2evans commented Dec 1, 2021

@schloerke if there's a problem that I can help speed-along with a manual push to my repo, let me know ... it seems that this set of GH actions is not behaving to your liking.

@schloerke
Copy link
Collaborator

@r2evans The GHA won't be able to push back changes to a forked repo (which is this PR's situation). GitHub says it is security issue when trying to push changes back to a forked repo from the main repo.

I'll be working on a better error message in the morning. I'll tag you when it's ready.

(Please don't fix the doc changes yet so I can continue testing in the morning)

@schloerke
Copy link
Collaborator

@r2evans The workflow has been updated. Please let me know if the new error message does not provide enough hints/instructions.

Since the workflow has been updated, please fix any errors it reports. Thank you!

@r2evans
Copy link
Author

r2evans commented Dec 1, 2021

Sorry @schloerke , I do not understand.

New commits:
+ 41b5c492b3f9dfb718b1e1ec03fa43665f54b01f `devtools::document()` (GitHub Actions)
Error: Some auto-generated commits were found. GHA is not allowed to push commits back to a forked repo.
Error: Please perform and commit these actions manually.
Error: Process completed with exit code 1.

While I understand that something in the routine GHA is internally updating the package documentation, why does that need to be pushed back to my branch in order to validate a PR here? It makes no sense to me.

Copy link
Collaborator

@schloerke schloerke left a comment

Choose a reason for hiding this comment

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

Similar to test-cancel.R, can you add tests to make sure limit and the return cancel function behave as expected? Thank you!

R/later_recurring.R Show resolved Hide resolved
R/later_recurring.R Outdated Show resolved Hide resolved
R/later_recurring.R Outdated Show resolved Hide resolved
R/later_recurring.R Show resolved Hide resolved
R/later_recurring.R Show resolved Hide resolved
R/later_recurring.R Outdated Show resolved Hide resolved
R/later_recurring.R Show resolved Hide resolved
- change limit= default to Inf (NA no longer accepted)
- add is_false (and one update to later.R)
- update doc (details and example code)
@schloerke
Copy link
Collaborator

Why does [the documentation changes] need to be pushed back to my branch in order to validate a PR here? It makes no sense to me.

** Growing pains, sorry.

The RoxygenNote field is getting updated from 7.1.0 to 7.1.2 in the DESCRIPTION file. You are correct in that those changes are not necessary for a valid R CMD check.

If you merge the latest from main (or rebase), the error should go away. Feel free to ignore it as well.

R/later_recurring.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@schloerke schloerke left a comment

Choose a reason for hiding this comment

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

Thank you for the tests and extra usage of is_false()!

@schloerke schloerke requested a review from wch December 1, 2021 19:55
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.

None yet

2 participants