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

SRC showing sessionInfo() and other header information #752

Closed
3 tasks done
Tracked by #69
donyunardi opened this issue Oct 7, 2022 · 6 comments · Fixed by #774
Closed
3 tasks done
Tracked by #69

SRC showing sessionInfo() and other header information #752

donyunardi opened this issue Oct 7, 2022 · 6 comments · Fixed by #774
Assignees
Labels

Comments

@donyunardi
Copy link
Contributor

donyunardi commented Oct 7, 2022

from @nikolas-burkoff

  • SRC currently shows sessionInfo (or at least versions of NEST packages installed) - we are losing this. Should we be?
    Edit: this might be fixed by Add sessionInfo #767

  • Also we are losing this option fixed by add hashing #774

  • We are also losing which library() calls we're using. Should we be? fixed by add hashing #774

@donyunardi
Copy link
Contributor Author

It doesn't have to in show R code but should be somewhere.

@nikolas-burkoff
Copy link
Contributor

Maybe look at putting it into teal rather than the modules would simplify things

@nikolas-burkoff nikolas-burkoff changed the title SRC showing sessionInfo() SRC showing sessionInfo() and other header information Oct 25, 2022
@Polkas
Copy link
Contributor

Polkas commented Nov 7, 2022

We have three separate problems here and they are very important for the reproducibility objective (the core of the NEST).
I feel we should invest here some time and make sure we are happy with solutions.

  • In my opinion already done part about session info could be improved with sessioninfo::session_info() or the renv lock file. Mostly nobody want to see the sessionInfo but to quickly reproduce the env so renv lock file could be the right direction.
    @nikolas-burkoff what you think?
  • So the "Show R Code" is not reproducible as no library calls are added by us now. This looks to be a part of teal.data and a feature of qenv.
  • The teal code option is a part of qenv in teal.code for me. Not obvious have to achieve it.

Is it clear I have to go to sessionInfo to reproduce the env and then run the "Show R Code"? We should think about some Message in the "Show R Code" modal that 'please make sure the environment is properly reproduce with XYZ'.

@pawelru

@Polkas
Copy link
Contributor

Polkas commented Nov 13, 2022

  1. As we are not adding libraries on the level of qenv, we do not know what packages are really needed.
    As a result we persist the old behavior of adding blindly all packages loaded in the R session (sessionInfo)

    teal/R/get_rcode_utils.R

    Lines 41 to 52 in ffb0421

    get_rcode_libraries <- function() {
    vapply(
    utils::sessionInfo()$otherPkgs,
    function(x) {
    paste0("library(", x$Package, ")")
    },
    character(1)
    ) %>%
    # put it into reverse order to correctly simulate executed code
    rev() %>%
    paste0(collapse = "\n")
    }
    I think we should create a separate issue for investigating it as it was not implemented or discussed here

  2. Important to remember that when I will use qenv alone it still not have proper libraries. I expect that the end user (developer) should provide all needed library calls and is able to do that, then we do not need this step of adding blindly all dependencies. Each module could have a list of needed dependencies which are added at the beginning to the qenv.

  3. Another thing is that I proposed improvements to Add sessionInfo #767 which was not commented. @nikolas-burkoff

  4. A small thing is that we should rename the

    get_rcode_str_install <- function() {
    function as is misleading.

@mhallal1 @donyunardi I reopen the issue till we will discuss it.

@Polkas Polkas reopened this Nov 13, 2022
@mhallal1
Copy link
Collaborator

mhallal1 commented Nov 14, 2022

  1. As we are not adding libraries on the level of qenv, we do not know what packages are really needed.
    As a result we persist the old behavior of adding blindly all packages loaded in the R session (sessionInfo)

    teal/R/get_rcode_utils.R

    Lines 41 to 52 in ffb0421

    get_rcode_libraries <- function() {
    vapply(
    utils::sessionInfo()$otherPkgs,
    function(x) {
    paste0("library(", x$Package, ")")
    },
    character(1)
    ) %>%
    # put it into reverse order to correctly simulate executed code
    rev() %>%
    paste0(collapse = "\n")
    }

    I think we should create a separate issue for investigating it as it was not implemented or discussed here
  2. Important to remember that when I will use qenv alone it still not have proper libraries. I expect that the end user (developer) should provide all needed library calls and is able to do that, then we do not need this step of adding blindly all dependencies. Each module could have a list of needed dependencies which are added at the beginning to the qenv.
  3. Another thing is that I proposed improvements to Add sessionInfo #767 which was not commented. @nikolas-burkoff
  4. A small thing is that we should rename the
    get_rcode_str_install <- function() {

    function as is misleading.

@mhallal1 @donyunardi I reopen the issue till we will discuss it.

@Polkas qenv is already merged on our main branch and we need to have the issues above covered asap for main users therefore we discussed and decided with @nikolas-burkoff to have those fixes for now, especially for the libraries. As this is an agile setup and the issue you suggest above is way more complex and requires a research issue by itself before implementation, please open a new issue with your points and the suggestions.

Issues opened for both points:

  1. Precise library calls in the Show R Code for each module #776
  2. sessionInfo() improvement #775

@Polkas
Copy link
Contributor

Polkas commented Nov 14, 2022

I edit the issues and now I feel content with closing it.
Another issue
linked to insightsengineering/teal.code#93

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

Successfully merging a pull request may close this issue.

4 participants