-
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
Environments, reproducibility, and consistent target validation #761
Comments
The specific problem that I encountered in this space was not false positives for invalid targets, but rather false negatives for valid targets (because of internal function changes). I think |
So maybe
I thought about diving into packages, global options, etc., but it gets tricky. If we depend too strongly on those details, workflows will become brittle and targets will almost never stay valid. If you want to manually add more dependencies, you can add to commands drake_plan(
x = {
dep1
dep2
actual(command(...))
}
) or use triggers. |
Regardless of where this goes in the long run, I think we really do need a |
My problem was similar to @ha0ye's. My thoughts on menus are they're good for infrequently encountered situations where you need help to make a decision. For a frequent situations you'll learn what you want and if you're locked into a menu with no way around it, it will quickly become annoying. You've provided a way around the menu with the options which is helpful. In my development workflow, which I've been loving for the most part, I've been calling make very frequently, so I can see the menu becoming annoying and I will almost certainly disable it. This would leave me back almost at square one. I say almost because at least now I am aware of the issue. But I'll still have no machinery provided by I've solved this problem for myself by building some IDE machinery that wraps up drake: https://github.com/MilesMcBain/spacemacs_cfg/blob/windows/private/ess/packages.el#L352. So now I never call
This makes me wonder if another approach to helping users with this overall issue is to provide some basic machinery that sets up their project in a way that adheres to convenient assumptions like this in combination with interactive helpers that ensure reproducibility. For example:
|
This sounds like a (almost off-topic, sorry for that. Quick idea on the phone) |
I did some looking into this, and at present it's not possible to restart an R session cleanly using the |
Comments
Good point. What about a less frequent default? Maybe once per session? I agree, a reminder to avert disaster defeats its own purpose if we overdo it.
Interesting. This where I thought @krlmlr intended to go with
Agreed. Also related: lockedata/starters#39 More brainstormingThese are all the major API functions prone to environment-related instability:
They all accept a What if EDITIt may be confusing to overload |
Some changes in b37ef00:
The more permanent solution will require more thought and care, and it will not be part of the upcoming version 7.0.0. This is fine since we are going for enhanced reproducibility without breaking changes. |
ProposalCreate API wrappers that look and act like Functions
Arguments
Behavior
Timing
CollaborationI am opening this issue up to the Chicago R Unconference, though I will probably not merge any branches until after version 7.0.0 is accepted to CRAN. |
I am trying to understand how my simple workflow would work with the callr API. As I mentioned I have a This file would build a config object with
|
As I picture it, your source("R/packages.R")
source("R/functions.R")
source("R/plan.R")
options(clustermq.scheduler = "slurm", clustermq.template = "custom.tmpl")
drake_config(
plan,
parallelism = "clustermq",
jobs = 4,
verbose = 6
) The There may be better ways to accomplish this, and I am open to suggestions. But I think the priorities are clear.
To me, the proposed R script follows naturally.
Glad you asked. |
FYI: just implemented #761 (comment) in #765. |
I know I promised that this would be an unconf issue, but I became so opinionated about the design that I thought it best to draft an implementation myself. I would still love feedback. A group discussion next week would be really helpful. |
Also, I am changing my mind about the timing of the rollout. The solution is new, but the problem is not. I have been thinking about environment-related pitfalls since the days of The I will still keep this issue open in order to encourage discussion over the next week. |
Thanks for the clarification @wlandau . I like the simplicity of the proposal 👍 Some things you could potentially discuss at the chirunconf:
I can tell you with my current project I have something like this going:
So then in my make.R I can have some generic code like this to load my R deps, while ignoring other R files: ## Find all the workflow steps and source them.
r_files <- list.files(path = "./",
full.names = TRUE,
pattern = "\\.R$",
recursive = TRUE)
workflow_files <- grep(pattern = "[0-9]{2}_",
x = r_files,
value = TRUE
)
lapply(workflow_files, source)
## remove working vars so as not to pollute global namespace.
rm(list = c('r_files', 'workflow_files')) |
Great points.
The API deliberately mimics functions
The
In my own projects, I often come back to the file structure now described in the chapter on .
+-- make.R # master script that calls make(). Intended for batch mode.
+-- _drake.R # configuration file to serve all the r_() functions
+-- R/
| +-- packages.R
| +-- functions.R
| +-- plan.R I think this could be enough for a function like |
Do you think |
For the record:
On Windows, |
Cool I follow the naming choice for Re And I think that's worth reflecting on because you're saying the |
True, I will reflect on it more. Explicit alignment with So then what intent should the prefix communicate? Reproducibility covers a huge range of topics and very strong scientific claims, so I am not sure it would be specific or appropriate enough to motivate this naming choice. On the other hand, we know exactly what it means to start a fresh R session. |
I usually use byobu (or similar tools as screen) for long running jobs. Theo point is that that you can close the terminal window and the command keeps running. You can later re-attach it. More flexible than QuestionHow can I get progress output (i.e. targets) when running Edit: My config for drake_config(plan, verbose = 2, targets = "benchmark_evaluation_report_diplodia", console_log_file = stdout(),
lazy_load = "promise", caching = "worker", template = list(log_file = "log-all%a.txt", n_cpus= 32),
garbage_collection = TRUE, jobs = 3, parallelism = "clustermq", force = T)
|
A mistake on my side, all good. The same applies to the * Option 'clustermq.scheduler' not set, defaulting to 'SLURM'
--- see: https://github.com/mschubert/clustermq/wiki/Configuration In the worker the |
Another thing that I just noticed is that |
Cool, thanks for the tip about
Unfortunately, the
Yeah, so that's an adjustment for sure. I tried to point it out here in the manual in the commented-out As for environment modules, I recommend putting them all in the template file (example).
Good point. |
Very cool, doing exactly this now!
Ok I see. So for debugging purposes I would do
|
Yes, for interactive debugging, everything must run in the same session. |
Re #761 (comment), I just took another look at the docs of |
Boom! Would it be possible to make this the default? I guess users would expect output in case they declare the verbosity in |
Good idea. Done in cf87fc5. |
All this functionality is now in version 7.0.0 on CRAN. We did not discuss the API decisions specifically (we mostly focused on this stuff) but I taught |
To Chicago R Unconference-goers
Let's talk about the API proposed here and documented here. Should the new functionality have different behavior? Better names?
Original post
The behavior of
make()
depends on the functions and data in your environment. If you change a function in the R console and then forget about it, you could falsely invalidate targets (prior discussions here, here, and here). This is a downside ofdrake
's deliberately extreme domain-specificity (discussed here).Temporary recommendations for users
make()
, it is good practice to start a fresh R session and be consistent in the way you load your packages and functions. I recommend that you keep a collection of setup scripts andsource()
them in a new session before doing anything serious (example here). Docker andcallr::r_vanilla()
may also help.outdated()
and maybedeps_profile()
before callingmake()
(dependency_profile()
indrake
version <= 6.2.1).envir <- new.env(parent = globalenv())
, load all your scripts there withsource(local = envir)
, and then callmake(envir = envir)
. This is advanced usage, and it is inconvenient in many cases.Thoughts on development
drake
will continue to have this problem as long as users are responsible for managing their R sessions. The most extreme and thorough solution would force every call tomake()
,outdated()
,vis_drake_graph()
, etc. to start a fresh session and run a bunch of R script files in a new, carefully controlled environment. This would be a major course correction, not impossible, but potentially fraught with major breaking changes and a loss of interactivity and domain-specificity.But what if
make()
were to prompt the user if some targets are outdated and the session is interactive? (Disabled withmake(force = TRUE)
oroptions(drake_force_interactive = TRUE)
.)@MilesMcBain, how much frustration would this kind of menu eliminate?
The text was updated successfully, but these errors were encountered: