-
Notifications
You must be signed in to change notification settings - Fork 128
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
A new interface for triggers #473
Comments
Also cc @kendonB. |
I'm not too sure about the syntax. I remember a discussion about a drake_plan(
target1 = drake_command(
get_data(file_in("data.csv")),
trigger(
command = FALSE,
logical = today() == "Tuesday"
)
),
...
) |
I like the idea and think @krlmlr's syntax is the tidiest. My only concern would be that introducing a change like this might make large plans very slow to start making. |
I like Kirill's approach too, mainly because it will be SO much easier to implement. I won't need to complicate the code analysis or sensitive internal bookkeeping. Also, there is a clear separation between the command and the trigger, which is great for users. The library(drake)
pkgconfig::set_config("drake::strings_in_dots" = "literals")
drake_plan(
x = target(
command = 1 + 1,
trigger = "always"
)
)
#> # A tibble: 1 x 3
#> target command trigger
#> * <chr> <chr> <chr>
#> 1 x 1 + 1 always
target(
command = 1 + 1,
trigger = "always"
)
#> # A tibble: 1 x 2
#> command trigger
#> <chr> <chr>
#> 1 1 + 1 always Question: should we detect dependencies from the
|
If we go Kirill's route (which I suggest we do), here are some implementation to-do's. These are done on the master branch:
And the rest will only exist on the i473 branch until it is ready to merge.
|
And of course, we'll want to elaborate on triggers, custom columns, and |
@kendonB, Kirill's approach will definitely be at least as fast to execute as what I had in mind, probably faster because there is less static code analysis. If we check the |
You could change the interface of Just thinking out loud. I'm slightly worried about the added complexity, especially in the light of brittle dependency detection for trigger conditions. I'd be very much interested in a "bare bones" version of drake that is restricted to evaluating commands and scheduling jobs, given the dependency graph and perhaps a run time estimate. Maybe this can be made available as a lower-level interface inside drake, or as a separate package? Complex triggers could then be implemented by automatically adding intermediate helper targets as necessary. (Error and progress messages would use artificial target names, though. Pretty sure there are other complications I'm not aware of.) |
A bare bones job scheduler is what I was going for with As for triggers, my original proposal would have added clutter. But with your idea, I feel there is a lot of gain and not much added complexity. |
Changing my mind about detecting dependencies from triggers. I think we should analyze the code of
|
I'll chime in that having |
I think I've got you covered, Alex. The way development is going, if you have a target |
Implemented via #478. The tests are very thorough, and it's on the master branch now. Let's keep iterating. I do not plan to submit to CRAN again until around August 10. |
@AlexAxthelm, please take a look at #480. I think the dependencies of triggers should be checked and processed beforehand, but I do not think changes to trigger-only dependencies should necessarily cause commands to rerun. Does the interface still do what you want? |
The problem
The current trigger interface is limiting. The choices "any", "always", "depends", "command", and "file" miss important use cases. For example, what if we want to check some remote data before deciding whether to build a target? For most projects, we would need to create a special upstream target with the fingerprint of the data and then have the real target depend on that fingerprint.
Proposal
It would be better to let users write arbitrary R code for each trigger.
Proposed
trigger()
function:People so far
@noamross brought up a motivating scenario in #252. @AlexAxthelm also has a good idea of what is needed because of his
drake
-powered work with databases. And of course, @krlmlr had the vision behind some of the best parts of the interface, including #232.Please let me know if I am forgetting anyone. If possible, it would be nice to have input from affected power users.
The text was updated successfully, but these errors were encountered: