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

Prohibit dynamic branching over unbranched dynamic files. #1302

Closed
4 tasks done
wlandau opened this issue Aug 7, 2020 · 4 comments
Closed
4 tasks done

Prohibit dynamic branching over unbranched dynamic files. #1302

wlandau opened this issue Aug 7, 2020 · 4 comments

Comments

@wlandau
Copy link
Member

wlandau commented Aug 7, 2020

Prework

  • Read and abide by drake's code of conduct.
  • Search for duplicates among the existing issues, both open and closed.
  • Be considerate of the maintainer's time and make it as easy as possible to troubleshoot any problems you identify. Read here and here to learn about minimal reproducible examples. Format your code according to the tidyverse style guide to make it easier for others to read.
  • If you think your issue has a quick and definite solution, consider posting to Stack Overflow under the drake-r-package tag. (If you anticipate extended follow-up and discussion, you are already in the right place!)

Description

https://stackoverflow.com/questions/63293094/how-do-i-read-dynamic-files-in-drake shows that if we dynamically branch over a dynamic file target, the file target must also use dynamic branching. We have two options for the example in https://stackoverflow.com/questions/63293094/how-do-i-read-dynamic-files-in-drake.

  1. Throw an informative error if import_paths is not dynamic.
  2. Invalidate all of data.

Because dynamic files are such a challenging and awkward architectural fit in drake, I am inclined to go with (1).

@wlandau wlandau changed the title Either prohibit or repair dynamic branching over unbranched dynamic files. Prohibit dynamic branching over unbranched dynamic files. Aug 7, 2020
@wlandau
Copy link
Member Author

wlandau commented Aug 7, 2020

Let's go with (1). What users actually want here is to invalidate some sub-targets but not others, and this is impossible in the Stack Overflow OP because of the composite hash.

@djbirke
Copy link

djbirke commented Aug 7, 2020

Thank you for following up on this! I still don't quite understand, and would like to ask more questions:

Do you want to allow the user to explicitly specify composite hashes? If yes, then option (2) allows them to do this, but how would they do that with option (1)?

If you do not want to allow users to explicitly specify composite hashes, wouldn't an alternative option (3) be to modify format = "file" such drake will track each of the target's elements on a file-by-file basis, making the code in OP work? I understand that tracking a large number of files can slow down drake, but in that case the user can simply not use format = "file". By using format = "file" the user would indicate intent to track files on a file-by-file basis.

@wlandau
Copy link
Member Author

wlandau commented Aug 7, 2020

I think the most pragmatic option is to add guardrails. Hashes are internal, so users should not have access. I hesitate to use file-specific hashes because of the potential burden on metadata storage, and the architectural challenges do not seem worth an incrementally more convenient UI.

@wlandau
Copy link
Member Author

wlandau commented Aug 7, 2020

By the way, dynamic branching over dynamic files should become much easier in targets after I implement ropensci/tarchetypes#5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants