stan-dev / posteriordb

Database with posteriors of interest for Bayesian inference
180 stars 36 forks source link

Fix typo in README #88

Closed eerolinna closed 4 years ago

eerolinna commented 4 years ago

Oops my editor removed trailing whitespace that is autogenerated by R and thus should be kept, I will add it back in a second

codecov-io commented 4 years ago

Codecov Report

Merging #88 into master will increase coverage by 20.96%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #88       +/-   ##
==========================================
+ Coverage   78.34%   99.3%   +20.96%     
==========================================
  Files          20       6       -14     
  Lines         651     144      -507     
==========================================
- Hits          510     143      -367     
+ Misses        141       1      -140
Impacted Files Coverage Δ
rpackage/R/gold_standard.R
rpackage/R/filter.R
rpackage/R/utils.R
rpackage/R/data_info.R
rpackage/R/utils_gold_standard.R
rpackage/R/write_pdb.R
rpackage/R/utils_tests.R
rpackage/R/tibble.R
rpackage/R/posterior.R
rpackage/R/model_code.R
... and 4 more

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 28d9539...f1e7fcd. Read the comment docs.

eerolinna commented 4 years ago

Ok now this is ready to be merged.

eerolinna commented 4 years ago

The CI failure is due to github rate limiting

GitHub API error (403): 403 Forbidden API rate limit exceeded for 35.202.145.110. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)

MansMeg commented 4 years ago

So the package fails. The reason is that apparently the Github PAT is not used in the PR so then the number of calls to the GitHub APi fails. Do you know how this can be fixed for PR?

eerolinna commented 4 years ago

I restarted the travis build which solved it for this PR. We might run into this again in the future, especially when the posterior database is bigger. It is possible to add GITHUB_PAT as a hidden environment variable to travis, however it seems that then PRs won't be able to use it, which partly defeats the point of doing it https://docs.travis-ci.com/user/environment-variables/#defining-variables-in-repository-settings

I guess we should wait and see if this problem occurs frequently and then figure something out if needed.

MansMeg commented 4 years ago

Indeed. I can also reduce the github tests in the R build.

eerolinna commented 4 years ago

Yeah that could be an option. We could have a small posterior database for testing that the github logic works as intended and then run the full tests only on a local database.

MansMeg commented 4 years ago

Exactly. I'll put this as an issue!

MansMeg commented 4 years ago

Now thi issue is fixed. Github tests are only run if a GITHUB_PAT is supplied.

eerolinna commented 4 years ago

Does CI have GITHUB_PAT?

MansMeg commented 4 years ago

Yes. Its a secure env variable for github access.

eerolinna commented 4 years ago

From what I understand PRs from other repos won't be able to use it (for security reasons, see https://docs.travis-ci.com/user/environment-variables/#defining-variables-in-repository-settings for more). I guess that is acceptable, we just have to keep it in mind.

Alternatively we could have a small sanity test for the remote pdb that gets run when GITHUB_PAT is not supplied. Something like getting model code and dataset for 8 schools posterior, that probably wouldn't cause too many API calls.

MansMeg commented 4 years ago

That could be reasonable. I'll add that as an issue. Each IP address has 60 calls per hour without a PR. But, for example, you can just add your PAT to your Travis and it will run the tests. I think the only problem is when people are doing PRs.