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

479 mirai lockfile@main #1263

Merged
merged 100 commits into from
Sep 23, 2024
Merged

479 mirai lockfile@main #1263

merged 100 commits into from
Sep 23, 2024

Conversation

gogonzo
Copy link
Contributor

@gogonzo gogonzo commented Jul 12, 2024

#1276

@pawelru @m7pr
This is a mirai alternative. Package seems to address the biggest issues reported during a research:

  • mirai has a native support of ExtendedTask
  • mirai is not being killed when runApp is executed (like callr does)
  • mirai by default opens a deamon in parallel R session without a need to handle the future::plan.
  • mirai has only one dependency in the whole dependency tree.

Disadvantages so far:

How does it work:

  • lockfile creation is invoked in init before application starts. This prevents to start the process each time when a new shiny session starts. Process is invoked as a promise and eventually teal_app.lock will be created
  • When shiny session starts download lockfile button is hidden by default. If promise is eventually resolved and lockfile is created then download button is shown.
  • alternatively, app developer can pre-compute lockfile and provide its path in teal.renv.lockfile option. In such case renv::snapshot will be skipped and user lockfile will be used in an app.

Logs and notifications

Logs are printed for app developer while notifications are presented to the app user:

  1. When app uses precomputed file:
  • log in init: Lockfile set using option "teal.renv.lockfile" - skipping automatic creation.
  • no notification to the app user.
  1. When app automatically determines snapshot:
  • log in init: Lockfile creation started based on { getwd() }.
  • log If lockfile created: Lockfile {path} containing { n-pkgs } packages created{ with errors or warnings }.
  • notification if lockfile created: Lockfile available to download
  • log if lockfile not created: Lockfile creation failed.
  • notification if lockfile not created: Lockfile creation failed.

m7pr and others added 30 commits July 1, 2024 12:13
Signed-off-by: Marcin <[email protected]>
Merge branch '479_callr_lockfile@main' of https://github.com/insightsengineering/teal into 479_callr_lockfile@main

# Conflicts:
#	R/teal_lockfile.R
Co-authored-by: Pawel Rucki <[email protected]>
Signed-off-by: Marcin <[email protected]>
Co-authored-by: Pawel Rucki <[email protected]>
Signed-off-by: Marcin <[email protected]>
Merge branch '479_callr_lockfile@main' of https://github.com/insightsengineering/teal into 479_callr_lockfile@main

# Conflicts:
#	R/teal_lockfile.R
@gogonzo
Copy link
Contributor Author

gogonzo commented Sep 19, 2024

@pawelru this is ready ⭐

vignettes/teal-options.Rmd Outdated Show resolved Hide resolved
R/module_teal_lockfile.R Outdated Show resolved Hide resolved
Copy link
Contributor Author

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

minor comments

R/module_teal_lockfile.R Outdated Show resolved Hide resolved
R/module_teal_lockfile.R Show resolved Hide resolved
R/module_teal_lockfile.R Outdated Show resolved Hide resolved
R/module_teal_lockfile.R Outdated Show resolved Hide resolved
R/module_teal_lockfile.R Outdated Show resolved Hide resolved
Copy link
Contributor

@m7pr m7pr left a comment

Choose a reason for hiding this comment

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

@pawelru since @gogonzo is the author, he can not approve the PR. I implemented all the changes we discussed and I am stating this is solid. Any final words?

vignettes/teal-options.Rmd Outdated Show resolved Hide resolved
Copy link
Contributor

@pawelru pawelru left a comment

Choose a reason for hiding this comment

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

Looks good 👍 it took us a while to get there :)

@m7pr
Copy link
Contributor

m7pr commented Sep 20, 2024

@gogonzo you can do the honors of merging this in : ) great colab team!

@gogonzo gogonzo merged commit 1b4bb50 into main Sep 23, 2024
29 checks passed
@gogonzo gogonzo deleted the 479_mirai_lockfile@main branch September 23, 2024 04:47
@github-actions github-actions bot locked and limited conversation to collaborators Sep 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants