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 scripting tasks config vignette #86

Merged
merged 26 commits into from
Dec 24, 2024
Merged

add scripting tasks config vignette #86

merged 26 commits into from
Dec 24, 2024

Conversation

zkamvar
Copy link
Member

@zkamvar zkamvar commented Nov 25, 2024

I've moved the scripting tasks configuration chapter of the user guide to here because as @annakrystalli correctly pointed out, it makes much more sense to link to this vignette where we can dynamically execute code.

That being said, I migrated this over and made the following changes:

  1. removed the screenshots and mostly redundant text
  2. added an overview of the structure of a tasks.json file (this is a variant of the cake-first approach)
  3. used the development version of pkgdown to incorporate Retain forward slashes in HTML img src paths r-lib/pkgdown#2811
  4. updated the pkgdown workflow to get the dev preview
  5. added a narrative throughout
  6. added a validation step

I had some challenges when writing this because the original script did not actually produce a valid configuration file (there were duplicate round IDs). There were also some minor issues that didn't really make sense once you put a narrative to it.

Specifically, why would we have two modeling tasks that have identical task IDs but share only a "mean" output type?

I was able to use the extra round to highlight the need for validation after write, but it would be good to get a modeler's eyes on this.

I'm also pretty sure I got the structure of the tasks.json wrong, but I am le tired. To do so, I created a list of names and used lobstr::tree() to create the tree diagram.

Copy link

github-actions bot commented Nov 25, 2024

@zkamvar zkamvar marked this pull request as ready for review December 6, 2024 20:41
Copy link
Member

@annakrystalli annakrystalli left a comment

Choose a reason for hiding this comment

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

Overall a great start with lots of useful demos and context. I've made quite a few suggestions in that name of clarifying a few things and I'd especially like to see the comments contained in code blocks moved to narrative text.

One really useful topic that hasn't been discussed is that the functions default to the latest schema version and demonstration of the use of hubAdmin.schema_version option to override this behaviour and protect workflows from breaking on new schema releases. I think this vignette would be an excellent place to discuss this.

vignettes/articles/scripting-tasks-config.Rmd Show resolved Hide resolved
vignettes/articles/scripting-tasks-config.Rmd Outdated Show resolved Hide resolved
vignettes/articles/scripting-tasks-config.Rmd Outdated Show resolved Hide resolved
vignettes/articles/scripting-tasks-config.Rmd Outdated Show resolved Hide resolved
vignettes/articles/scripting-tasks-config.Rmd Outdated Show resolved Hide resolved
vignettes/articles/scripting-tasks-config.Rmd Outdated Show resolved Hide resolved
vignettes/articles/scripting-tasks-config.Rmd Show resolved Hide resolved
vignettes/articles/scripting-tasks-config.Rmd Outdated Show resolved Hide resolved
vignettes/articles/scripting-tasks-config.Rmd Outdated Show resolved Hide resolved
vignettes/articles/scripting-tasks-config.Rmd Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.88%. Comparing base (b809161) to head (f5ae2b1).
Report is 51 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
- Coverage   89.24%   88.88%   -0.36%     
==========================================
  Files          29       29              
  Lines        2185     2322     +137     
==========================================
+ Hits         1950     2064     +114     
- Misses        235      258      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zkamvar and others added 13 commits December 11, 2024 08:38
@zkamvar zkamvar force-pushed the znk/task-config-vignette branch from de2c750 to 90f6018 Compare December 11, 2024 16:38
@zkamvar
Copy link
Member Author

zkamvar commented Dec 11, 2024

a note about the force push: I rebased this branch onto the main branch.

A weird bug just popped up and I don't know where it's coming from, but
effectively, when a value with a class of "error" is passed to
`encodeString()`, it fails with:

Error in UseMethod("conditionMessage") :
  no applicable method for 'conditionMessage' applied to an object of class "error"

This never used to happen, so I'm fixing it here.
Copy link
Member

@annakrystalli annakrystalli left a comment

Choose a reason for hiding this comment

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

Thanks for responding to all the previous comments! I have a couple of minor additional comments/suggestions.

However, the more I thought about the example of adding a second round, the more I felt we need to modify it. I think the second round is both a bit unrealistic but more importantly probably bad practice to demonstrate. While it does pass validation and is technically allowed, we are repeating round IDs in the second round that exist in the first but just getting round this by setting round_id_from_variable: false. This doesn't feel like something we want to encourage in our docs.

Instead why don't we create a whole new round with new values in origin date but largely reusing previously created objects and code and a different submission window (still relative to origin date) due to...(some reason, any good ideas?). Then we could append the round via append_round(). I know this will add a lot of repeated code to the vignette but I think this would still be preferable because:

  • it avoids demonstrating hacky practice
  • might be more realistic
  • demonstrates the value of re-using previous code and objects created
  • demonstrates the use of append_round()

For a validation error, you could leave a round_id that already exists in the previous round's origin_date, catch it with the validation and correct it.

vignettes/articles/scripting-tasks-config.Rmd Outdated Show resolved Hide resolved
vignettes/articles/scripting-tasks-config.Rmd Outdated Show resolved Hide resolved
vignettes/articles/scripting-tasks-config.Rmd Outdated Show resolved Hide resolved
vignettes/articles/scripting-tasks-config.Rmd Outdated Show resolved Hide resolved
@zkamvar
Copy link
Member Author

zkamvar commented Dec 24, 2024

However, the more I thought about the example of adding a second round, the more I felt we need to modify it. I think the second round is both a bit unrealistic but more importantly probably bad practice to demonstrate. While it does pass validation and is technically allowed, we are repeating round IDs in the second round that exist in the first but just getting round this by setting round_id_from_variable: false. This doesn't feel like something we want to encourage in our docs.

Trying to work around duplicated round IDs was not my intention, but now that you mention it, I can see why it seems that way. My intention was to demonstrate a stand-alone round (hence round_id_from_variable: false).

I think your suggestions make sense, but instead of adding another round of review, I am going to cut off that last section and open an issue to amend the vignette. This vignette will replace the scripting tasks configuration chapter from the hubverse documentation, which currently will produce an invalid configuration file. The vignette without that last section is still not perfect, but it's better than what we have at the moment.

This section had brought up concerns [1] about the kind of scripting
behaviour this was encouraging. I have decided to remove it to move this
forward [2]. To incorporate a section in here, the narrative needs to be
changed so that it has a clear goal that a hub administrator can
identify with.

[1]: #86 (review)
[2]: #86 (comment)
@zkamvar
Copy link
Member Author

zkamvar commented Dec 24, 2024

Note: there is a small bug in pillar that's causing weird output: r-lib/pillar#720, specifically, the section demonstrating an error in creation of a target metadata item has this error from pillar.

#> Error in get(paste0(generic, ".", class), envir = get_method_env()) : 
#>   object 'type_sum.accel' not found

Given that the CRAN team are on break, it may be mid-January until it's fixed.

Copy link
Contributor

@elray1 elray1 left a comment

Choose a reason for hiding this comment

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

This is a great thing to have, and overall very clear! I made a few fairly minor comments throughout.

Copy link
Contributor

@elray1 elray1 left a comment

Choose a reason for hiding this comment

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

this looks good enough to merge in to me, with plans to revisit the example soon.

@zkamvar zkamvar dismissed annakrystalli’s stale review December 24, 2024 20:27

See #86 (comment).

I have addressed the minor issues with this, but the narrative issues require a deeper rewrite. Since the goal of this was to get a "good enough" replacement for the currently broken scripting tasks config chapter, I am going to push this through and open an issue with the concerns that everyone (myself included) raised about the narrative structure (or lack thereof).

@zkamvar zkamvar merged commit e65953d into main Dec 24, 2024
8 checks passed
@zkamvar zkamvar deleted the znk/task-config-vignette branch December 24, 2024 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants