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

introducing rstudio addin for dso repro stage #19

Merged
merged 17 commits into from
Nov 11, 2024
Merged

Conversation

DSchreyer
Copy link
Collaborator

v0.5.1

New Features

  • Introduced the rstudio addin to dso repro the current stage

@DSchreyer DSchreyer requested review from grst and tschwarzl October 25, 2024 09:14
@DSchreyer
Copy link
Collaborator Author

local testing required

R/addin.R Outdated
dvc_yaml_path <- file.path(stage_path, "dvc.yaml")

tryCatch({
message(glue::glue("Reproducing the current stage"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Add to "#' @importFrom glue glue" for roxygen to resolve import dependencies

R/addin.R Outdated
message(glue::glue("Report generated: {report_file}"))
message(glue::glue("Displaying report file: {report_file}"))
# Check the file extension and display in the viewer
rstudioapi::viewer(report_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

import / importFrom with rstudioapi

Copy link
Collaborator

Choose a reason for hiding this comment

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

technically not necessary when using ::.
Afaik the pre-commit checks also ensure that one does not forget to add it to DESCRIPTION

Copy link
Contributor

@tschwarzl tschwarzl left a comment

Choose a reason for hiding this comment

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

Very nice.

I would like to like discuss an option to add
"dso repro stage" - with dependencies
"dso repro stage -s" - only single stage
"dso repro stage -f" - force.

R/addin.R Outdated
message(glue::glue("Report generated: {report_file}"))
message(glue::glue("Displaying report file: {report_file}"))
# Check the file extension and display in the viewer
rstudioapi::viewer(report_file)
Copy link
Collaborator

Choose a reason for hiding this comment

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

technically not necessary when using ::.
Afaik the pre-commit checks also ensure that one does not forget to add it to DESCRIPTION

CHANGELOG.md Show resolved Hide resolved
R/addin.R Outdated
result <- system2(DSO_EXEC,
c("repro -s", shQuote(dvc_yaml_path)))
if (result != 0) {
stop("System command failed with status: {result}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

glue seems to be missing here

@DSchreyer
Copy link
Collaborator Author

This hooks will take care of running consistency checks and automatically 
syncing dvc.
Do you want to install the pre-commit hooks now? [y/n]: Aborted.```

pre commit hooks lead to error which is not identified and can not be resolved within rstudio and the current addin

@grst
Copy link
Collaborator

grst commented Nov 6, 2024

So do I get this right that not the actual hooks are the problem, but dso asking whether to install the hooks?

@DSchreyer
Copy link
Collaborator Author

So do I get this right that not the actual hooks are the problem, but dso asking whether to install the hooks?

correct. DSO asks to installl but the addin is not interactive and therefore it results in an error. When the hooks are installed or dso doesnt ask then it works normally

@grst
Copy link
Collaborator

grst commented Nov 6, 2024

ok, then I propose adding an ENV var or command line flag to the dso CLI to not ask that can be used from R

@DSchreyer
Copy link
Collaborator Author

[11/07/24 10:10:47] INFO Compiling a total of 29 config files.
[11/07/24 10:10:49] INFO Configuration compiled successfully.
INFO Running dvc repro -s <stagename>/dvc.yaml/dvc.yaml

Another issue arose: recursive dvc.yaml addition to path

@DSchreyer
Copy link
Collaborator Author

Fixed the issue named above and removed the show report function. The show report function froze the session when it was too large. Report can be opened manually inside Rstudio

@DSchreyer DSchreyer requested a review from tschwarzl November 7, 2024 12:09
@DSchreyer DSchreyer requested a review from grst November 7, 2024 12:09
Copy link
Collaborator

@grst grst left a comment

Choose a reason for hiding this comment

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

just two nitpicky things left; LGTM

CHANGELOG.md Outdated

### Fixes

- Removed ... in reload function
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Removed ... in reload function
- Removed `...` in reload function

R/addin.R Outdated
#' }
dso_repro_stage_addin <- function() {
check_stage_here()
dvc_yaml_path <- stage_here("dvc.yaml")
Copy link
Collaborator

@grst grst Nov 11, 2024

Choose a reason for hiding this comment

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

dvc_yaml_path doesn't seem to be used?

@DSchreyer DSchreyer merged commit 550d921 into main Nov 11, 2024
4 of 5 checks passed
@DSchreyer DSchreyer deleted the rstudio_addin branch November 11, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants