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

Use faster retrieval of remote information for GitHub remotes #142

Merged
merged 4 commits into from Aug 24, 2018

Conversation

@jimhester
Copy link
Member

@jimhester jimhester commented Aug 20, 2018

remote_package_name.github_remote and remote_sha.github_remote will
lookup package name locally if possible and use faster REST APIs if the curl
package is installed. This makes the remote lookup about 7-10x faster when the
package to be installed has not changed since the last install.

pkgload::load_all("~/p/remotes")
remote <- github_remote("r-lib/devtools")
bnch <- bench::press(
  use_curl = c(FALSE, TRUE),
  use_local = c(FALSE, TRUE),
  bench::mark(
    get_shas = {
    package_name <- remote_package_name(remote, use_local = use_local, use_curl = use_curl)
    local_sha <- local_sha(package_name)
    remote_sha <- remote_sha(remote, local_sha, use_curl = use_curl)
  }, iterations = 3,
    filter_gc = FALSE
  )
)
#> Running with:
#>   use_curl use_local
#> 1 FALSE    FALSE
#> 2 TRUE     FALSE
#> 3 FALSE    TRUE
#> 4 TRUE     TRUE

bnch[c(2:10)]
#> # A tibble: 4 x 9
#>   use_curl use_local     min    mean  median   max `itr/sec` mem_alloc
#>   <lgl>    <lgl>     <bch:t> <bch:t> <bch:t> <bch>     <dbl> <bch:byt>
#> 1 FALSE    FALSE     563.8ms 606.8ms 586.1ms 670ms      1.65    4.65MB
#> 2 TRUE     FALSE     160.9ms 273.5ms 165.6ms 494ms      3.66  206.08KB
#> 3 FALSE    TRUE      269.7ms 273.1ms 269.9ms 280ms      3.66   19.37KB
#> 4 TRUE     TRUE       74.1ms  86.5ms  84.7ms 101ms     11.6    19.12KB
#> # ... with 1 more variable: n_gc <dbl>
summary(bnch, relative = TRUE, filter_gc = FALSE)[c(2:10)]
#> # A tibble: 4 x 9
#>   use_curl use_local   min  mean median   max `itr/sec` mem_alloc  n_gc
#>   <lgl>    <lgl>     <dbl> <dbl>  <dbl> <dbl>     <dbl>     <dbl> <dbl>
#> 1 FALSE    FALSE      7.61  7.01   6.92  6.65      1       249.     Inf
#> 2 TRUE     FALSE      2.17  3.16   1.96  4.90      2.22     10.8    NaN
#> 3 FALSE    TRUE       3.64  3.16   3.19  2.78      2.22      1.01   NaN
#> 4 TRUE     TRUE       1     1      1     1         7.01      1      NaN
@jimhester
Copy link
Member Author

@jimhester jimhester commented Aug 20, 2018

@gaborcsardi My main question with this is the use_local argument. This assumes by default that if a package is installed locally with the same name as the remote repo name, then that is the remote repo's package name. I think this will be correct the vast majority of the time, but it could be a false positive if the remote repo is actually a different name. Therefore I think we need to have a user visible way to turn it off, but I think it will be rare enough I am not sure it is worth complicating the regular API to provide a regular option for it. What do you think about just letting it be set by an environment variable or global option?

Loading

@codecov-io
Copy link

@codecov-io codecov-io commented Aug 20, 2018

Codecov Report

No coverage uploaded for pull request base (master@6a8884d). Click here to learn what that means.
The diff coverage is 57.44%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #142   +/-   ##
=========================================
  Coverage          ?   86.86%           
=========================================
  Files             ?       29           
  Lines             ?     1614           
  Branches          ?        0           
=========================================
  Hits              ?     1402           
  Misses            ?      212           
  Partials          ?        0
Impacted Files Coverage Δ
R/github.R 15.68% <0%> (ø)
R/parse-git.R 100% <100%> (ø)
R/install-bioc.R 91.26% <100%> (ø)
R/install-github.R 55.17% <18.18%> (ø)
R/install-git.R 91.46% <50%> (ø)
R/install-remote.R 82.4% <50%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a8884d...0d73628. Read the comment docs.

Loading

@gaborcsardi
Copy link
Contributor

@gaborcsardi gaborcsardi commented Aug 20, 2018

So, the thing is, in pkgdepends (and co.) if the package name is not the same as the repo name, you need to include the package name in the remote name. It looks like this: https://github.com/r-lib/pkgdepends/blob/ed31a93b514c023f80ba617dedc2672f837d8271/tests/testthat/test-parse-remotes.R#L167-L170

We could maybe just include this in remotes as well? Maybe together with your heuristic, so there won't be too much breakage?

Loading

@jimhester
Copy link
Member Author

@jimhester jimhester commented Aug 20, 2018

Yeah that sounds good to me!

Loading

@jimhester
Copy link
Member Author

@jimhester jimhester commented Aug 21, 2018

Ok I think this is ready for review now, we now support pkg=user/repo syntax like pkgdepends and I added some tests for the new code paths.

Loading

@jimhester
Copy link
Member Author

@jimhester jimhester commented Aug 23, 2018

@gaborcsardi Any additional comments on this?

Loading

Copy link
Contributor

@gaborcsardi gaborcsardi left a comment

Looks good!

What happens is we assume some package name based on the local name, but it is actually wrong?

Loading

R/github.R Outdated
} else {
tmp <- tempfile()
on.exit(unlink(tmp))
Copy link
Contributor

@gaborcsardi gaborcsardi Aug 23, 2018

Choose a reason for hiding this comment

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

I think add = TRUE is good practice in general.

Loading

R/github.R Outdated
rawToChar(res$content)
} else {
tmp <- tempfile()
on.exit(unlink(tmp))
Copy link
Contributor

@gaborcsardi gaborcsardi Aug 23, 2018

Choose a reason for hiding this comment

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

add = TRUE

Loading

Copy link
Member Author

@jimhester jimhester Aug 23, 2018

Choose a reason for hiding this comment

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

You only need add = TRUE if there is another on.exit() call in the same scope no? I guess add = TRUE unconditionally is safer, I can add it.

Loading

@@ -41,33 +39,27 @@
#' install_github("hadley/private", auth_token = "abc")
#'
#' }
install_github <- function(repo, username = NULL,
install_github <- function(repo,
Copy link
Contributor

@gaborcsardi gaborcsardi Aug 23, 2018

Choose a reason for hiding this comment

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

Removing the username argument should be the NEWS, I think.

Loading


params
}

Copy link
Contributor

@gaborcsardi gaborcsardi Aug 23, 2018

Choose a reason for hiding this comment

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

Is this all just the pkgdepends code? Or new code? Dont' mind, just curious.

Loading

Copy link
Member Author

@jimhester jimhester Aug 23, 2018

Choose a reason for hiding this comment

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

It is code from remotes, I just moved it out of install-git.r to a separate file to make testing it separately easier.

Loading

Copy link
Member Author

@jimhester jimhester Aug 23, 2018

Choose a reason for hiding this comment

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

Sorry for the bigger diff because of this :(

Loading

Copy link
Contributor

@gaborcsardi gaborcsardi Aug 23, 2018

Choose a reason for hiding this comment

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

OK, np.

Loading

@jimhester
Copy link
Member Author

@jimhester jimhester commented Aug 23, 2018

I think the worst that could happen if we assume the wrong name is the local sha would be from the wrong package, so the package would be re-installed when it may not have had to be.

Git's SHA1s should prevent spurious collisions between the wrong packages even if we guess the name wrong and lookup the local sha from the wrong package, so I believe we should never get a false negative and fail to install something when we should.

Loading

@gaborcsardi
Copy link
Contributor

@gaborcsardi gaborcsardi commented Aug 23, 2018

So this is all good now, I think, right?

Loading

`remote_package_name.github_remote` and `remote_sha.github_remote` will
lookup package name locally if possible and use faster REST APIs if the curl
package is installed. This makes the remote lookup about 7-10x faster when the
package to be installed has not changed since the last install.

    pkgload::load_all("~/p/remotes")
    remote <- github_remote("r-lib/devtools")
    bnch <- bench::press(
      use_curl = c(FALSE, TRUE),
      use_local = c(FALSE, TRUE),
      bench::mark(
        get_shas = {
        package_name <- remote_package_name(remote, use_local = use_local, use_curl = use_curl)
        local_sha <- local_sha(package_name)
        remote_sha <- remote_sha(remote, local_sha, use_curl = use_curl)
      }, iterations = 3,
        filter_gc = FALSE
      )
    )
    #> Running with:
    #>   use_curl use_local
    #> 1 FALSE    FALSE
    #> 2 TRUE     FALSE
    #> 3 FALSE    TRUE
    #> 4 TRUE     TRUE

    bnch[c(2:10)]
    #> # A tibble: 4 x 9
    #>   use_curl use_local     min    mean  median   max `itr/sec` mem_alloc
    #>   <lgl>    <lgl>     <bch:t> <bch:t> <bch:t> <bch>     <dbl> <bch:byt>
    #> 1 FALSE    FALSE     563.8ms 606.8ms 586.1ms 670ms      1.65    4.65MB
    #> 2 TRUE     FALSE     160.9ms 273.5ms 165.6ms 494ms      3.66  206.08KB
    #> 3 FALSE    TRUE      269.7ms 273.1ms 269.9ms 280ms      3.66   19.37KB
    #> 4 TRUE     TRUE       74.1ms  86.5ms  84.7ms 101ms     11.6    19.12KB
    #> # ... with 1 more variable: n_gc <dbl>
    summary(bnch, relative = TRUE, filter_gc = FALSE)[c(2:10)]
    #> # A tibble: 4 x 9
    #>   use_curl use_local   min  mean median   max `itr/sec` mem_alloc  n_gc
    #>   <lgl>    <lgl>     <dbl> <dbl>  <dbl> <dbl>     <dbl>     <dbl> <dbl>
    #> 1 FALSE    FALSE      7.61  7.01   6.92  6.65      1       249.     Inf
    #> 2 TRUE     FALSE      2.17  3.16   1.96  4.90      2.22     10.8    NaN
    #> 3 FALSE    TRUE       3.64  3.16   3.19  2.78      2.22      1.01   NaN
    #> 4 TRUE     TRUE       1     1      1     1         7.01      1      NaN
@jimhester jimhester merged commit 022537c into r-lib:master Aug 24, 2018
4 checks passed
Loading
@jimhester
Copy link
Member Author

@jimhester jimhester commented Aug 24, 2018

Thank you for the review!

Loading

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

Successfully merging this pull request may close these issues.

None yet

3 participants