Seabbs/issue199

[seabbs]

This PR closes #199 by adding {touchstone} benchmarks using the package script examples. See #238 for a test implementation.

[seabbs]

The reason this is failing at the moment is because I believe it has to have a version on the target branch in order to work (so heredevelop.

[adrian-lison]

The reason this is failing at the moment is because I believe it has to have a version on the target branch in order to work (so heredevelop.

So you think it will work after we have merged this in develop? If we are scared, we could also create another test branch from this one here, make some changes and test a pull request from the test branch into this one.

[seabbs]

As suggested by @adrian-lison this is now being tested in #203

[codecov-commenter]

Codecov Report

Merging #200 (a80c100) into develop (d21ab56) will decrease coverage by 0.05%.
The diff coverage is 97.29%.

❗ Current head a80c100 differs from pull request most recent head ee94c48. Consider uploading reports for the commit ee94c48 to get more accurate results

@@             Coverage Diff             @@
##           develop     #200      +/-   ##
===========================================
- Coverage    97.31%   97.27%   -0.05%     
===========================================
  Files           14       14              
  Lines         1527     1540      +13     
===========================================
+ Hits          1486     1498      +12     
- Misses          41       42       +1     

Impacted Files	Coverage Δ
R/formula-tools.R	95.00% <96.15%> (-0.24%)	⬇️
R/check.R	100.00% <100.00%> (ø)
R/model-design-tools.R	100.00% <100.00%> (ø)
R/model-tools.R	99.20% <100.00%> (ø)
R/preprocess.R	96.44% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[seabbs]

Proving to be a bit of a fiddle to get right.

[adrian-lison]

@seabbs I looked into this now and I agree it seems that the current point of failure is really the function pin_assets. It works neither when just using "touchstone" or just using "inst/examples", and the failure happens somewhere in the call to pin_assets (see here), which then triggers the following error from the cli package (see here):

Error in vapply(X, FUN, FUN.VALUE = character(1), ..., USE.NAMES = USE.NAMES) : 
  values must be type 'character',
 but FUN(X[[1]]) result is type 'list'
Calls: <Anonymous> ... transform -> make_link -> make_link_file -> vcapply -> vapply

I haven't figured out yet which call exactly it is (maybe more deep down the hierarchy, because all the direct calls to cli in the function seem alright) and what is causing this.

One general guess though I had might be versioning of {touchstone}. At the moment, we have this in the workflow
- uses: lorenzwalthert/touchstone/actions/receive@main
but maybe we have to specify the correct version or use the v1 wildcard: https://github.com/lorenzwalthert/touchstone/blob/main/actions/README.md ?

[adrian-lison]

My best guess is that one or several of the directories that we want to pin cannot be found and then it is doing something really weird when trying to write a warning on this using cli, see here

if (!all(valid_dirs)) {
    cli::cli_warn(paste0(
      "The following asset{?s} could not be found and will ",
      "not be copied: {.path {dirs[!valid_dirs]}}"
    ))
  }

somehow, if trying to format a path which is not a character but a list, then we get exactly the error we have. But I have no idea how this could happen, I cannot reproduce at this function level.

[adrian-lison]

@seabbs I noticed that {touchstone} is recently also failing with the same error in a pull request in EpiNow2 and that @sbfnk already played with removing the pin_assets function as well. @sbfnk any idea why pin_assets is not working anymore or how to get around it?

[adrian-lison]

Tried a workaround that avoids pin_assets by sourcing the relevant files directly (works as long they agree in all branches), but now we get new errors. Currently I am stuck with this one. Very weird.

[seabbs]

but maybe we have to specify the correct version or use the v1 wildcard:

Really good point about using standard versions. This seems like it might well be it (especially due to the change in EpiNow2 from working -> not.

somehow, if trying to format a path which is not a character but a list, then we get exactly the error we have. But I have no idea how this could happen, I cannot reproduce at this function level.

Great job digging and yeah I have no idea what is happening here.

By the way for local testing I have been finding act really helpful for GitHub Actions: https://github.com/nektos/act

I was a bit confused where we were so just did a quick revert back to a version with fixed version labels.

[seabbs]

ut now we get new errors. Currently I am stuck with this one. Very weird.

This is very weird

[seabbs]

😱

[seabbs]

@adrian-lison or @sbfnk any luck sorting {touchstone} out or opening an issue with the creator? I think not but will check around and then make an issue on the repository to get this sorted.

[seabbs]

All finally working now so just a matter of fine-tuning what we want to run in the benchmarks. Annoyingly in needs to recompile the model each time and so this slows everything down. It doesn't seem like we can avoid this either.

[seabbs]

This all appears to be working and I think the current benchmarks are a good MVP. Planning on merging as a self-review and we can iterate.