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

Add CI builds on Github Actions #2544

Merged
merged 22 commits into from
Apr 28, 2020
Merged

Add CI builds on Github Actions #2544

merged 22 commits into from
Apr 28, 2020

Conversation

infotroph
Copy link
Member

@infotroph infotroph commented Mar 5, 2020

Description

First pass at a continuous integration pipeline using GitHub Actions. These do not yet replace our existing Travis CI build.

Runs automatically on every push, and does the following:

  • Builds and installs the core PEcAn packages inside the depends Docker image. It does not spin up the whole Docker-compose stack, just uses the image to avoid reinstalling dependencies.
  • Sets up a test instance of BeTY using the pecan/db:ci Docker image, which contains the records from the most recent successful dumps of servers 0, 1, 2, 5.
  • Runs all package tests, including those that require database access,
  • Runs R CMD check on all packages, and breaks the build for any newly added ERRORs, WARNINGs or NOTEs. As on Travis, issues that existed before September 2019 are grandfathered in and do not produce warnings.
  • Performs a simple integration test using SIPNET
  • Checks for unexpected changes to tracked files (90% of the time this means you forgot to run Roxygen before committing)

You can see the results of each run in the Actions tab. My understanding is that when you fork PEcAn these actions will run in your own fork too with no opt-in needed.

Motivation and Context

Closes #2412. As discussed there, the single biggest apeal of Actions is that its runtime limits are much more generous than the 50-minute ceiling we often hit on Travis. Actions is also very flexible and well-integrated with the rest of GitHub; if this workflow works well, it's easy to envision adding
more actions that interface with issues and PRs, build Docker images, run more elaborate integration tests at longer time intervals, and so on.

Since many of these changes will be relevant to possible GSoC projects this coming summer, I'd like to encourage potential applicants to join the review process for this PR. To encourage discussion, I'll keep this open for a while to make sure everyone has lots of chances to comment before it's merged. I've marked it as work in progress to avoid accidental merging while discussion is still ongoing.

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

The "${HOME}${R_LIBS#'~'}" is to manually expand tilde in R_LIBS:
* GitHub sets env vars as unprocessed strings, so it won't be expanded there.
* POSIX shell expansion rules say ~ is always replaced *before* expanding $,
so `FOO='~'; mkdir -p $FOO` creates a dir literally named '~'.
@infotroph infotroph requested a review from robkooper March 5, 2020 19:47
@rahul799
Copy link
Member

rahul799 commented Mar 6, 2020

Travis is failing due to tmvtnorm package. To be more precise, it is not able to download one of the dependencies mvtnorm.

Error: (converted from warning) package ‘mvtnorm’ was built under R version 3.6.3
Execution halted
ERROR: lazy loading failed for package ‘tmvtnorm’
* removing ‘/home/travis/R/Library/tmvtnorm’
Error in i.p(...) : 
  (converted from warning) installation of one or more packages failed,
  probably ‘tmvtnorm’

The error is caused due to the version mismatch as Travis is using R 3.6.2 & it is trying to load the latest version of mvtnorm e.g., 3.6.3

@infotroph
Copy link
Member Author

Thanks for investigating, @rahul799. Apparently c2d4u, which we use for precompiled binaries of some R packages, has already started updating some of their binaries to R 3.6.3 but is still serving others compiled with R 3.6.2.As far as I can tell, c2d4u's philosophy is that "compiled packages are usually compatible between patch versions of R, so mixing them is fine", but R always warns any time you load a package built for a different version -- and what we see here is the package installation process for tmvtnorm hitting such a warning when it loads the already-installed mvtnorm, then (correctly) elevating the warning to an error.

Version issues like this have been an ongoing pain point for a while and we think they'll be easier to improve on Actions than on Travis, which is part of why we'd like to migrate when we can.

As a short-term fix, I've abused Travis's caching abilities by using some API calls to reinstall a 3.6.2-built version of mvtnorm and then restarted the build.

@shivanshu1086
Copy link


ERROR: lazy loading failed for package ‘tmvtnorm’
* removing ‘/home/travis/R/Library/tmvtnorm’
Error in i.p(...) : 
  (converted from warning) installation of package ‘tmvtnorm’ had non-zero exit status
Calls: <Anonymous> ... with_rprofile_user -> with_envvar -> force -> force -> i.p
Execution halted
Makefile:130: recipe for target '.doc/modules/emulator' failed
make: *** [.doc/modules/emulator] Error 1
The command "scripts/travis/script.sh" exited with 1.

We need to either update the R version of the Travis CI to 3.6.3 or either we can go to the 3.6.2 version of mvtnorm.

@infotroph
Copy link
Member Author

infotroph commented Mar 6, 2020

After I put a rebuilt mvtnorm into the build cache, this PR is currently passing on Travis. Let's now try to focus this thread on evaluating the Actions code rather than the Travis output -- Travis doesn't run any of the Actions code at all, so we'll have to notice the bugs ourselves!

@shivanshu1086
Copy link

Thanks for correcting me @infotroph!

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
infotroph and others added 3 commits March 7, 2020 10:31
Co-Authored-By: Rahul Agrawal <41531498+rahul799@users.noreply.github.com>
Co-Authored-By: Rahul Agrawal <41531498+rahul799@users.noreply.github.com>
Co-Authored-By: Rahul Agrawal <41531498+rahul799@users.noreply.github.com>
.github/workflows/ci.yml Outdated Show resolved Hide resolved
infotroph and others added 2 commits March 7, 2020 10:35
Co-Authored-By: Rahul Agrawal <41531498+rahul799@users.noreply.github.com>
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@ashiklom ashiklom left a comment

Choose a reason for hiding this comment

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

This is fantastic!! The way you cache R packages with the R_LIBS variable is really clever.

In a future follow-up PR, we should switch to using the "official" PEcAn images (i.e. not infotroph/pecan-depends). And in another future PR, we should test the Docker compose installation instructions.

I think having GH Actions-based integration testing will be a major milestone for PEcAn development, and this is a critical first step to that. I'm excited.

@infotroph
Copy link
Member Author

In a future follow-up PR, we should switch to using the "official" PEcAn images (i.e. not infotroph/pecan-depends)

100% agree -- we can probably even do it in this PR if #2538 is merged before this one

@robkooper
Copy link
Member

How hard is it to make an action that runs on a nightly basis to create a new pecan/depends:develop image?

@infotroph
Copy link
Member Author

How hard is it to make an action that runs on a nightly basis to create a new pecan/depends:develop image?

Probably not too hard. I think several folks have already written that into their GSoC proposal drafts, so I'm happy waiting and letting them do it :)

@infotroph infotroph changed the title WIP: Add CI builds on Github Actions Add CI builds on Github Actions Apr 20, 2020
@infotroph infotroph changed the title Add CI builds on Github Actions WIP: Add CI builds on Github Actions Apr 21, 2020
@infotroph infotroph changed the title WIP: Add CI builds on Github Actions Add CI builds on Github Actions Apr 21, 2020
@infotroph
Copy link
Member Author

This appears to be fully working and is ready for final review

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@infotroph
Copy link
Member Author

One more change! Switched test database from the static dump file used on Travis (last updated three or four years ago; takes 4-5 minutes to download and read in) to the pecan/db:ci image (stays up-to-date; takes 2-3 minutes to download and read in). A worthy goal for a future cleanup round: Reduce the number of records needed for testing so that the ci image can become even smaller yet.

@infotroph
Copy link
Member Author

@mdietze @robkooper OK, now this is ready for final review and merge. Please squash on merging -- the intermediate commits are all debugs and typo fixes.

@robkooper robkooper merged commit bb6bde5 into develop Apr 28, 2020
@robkooper robkooper deleted the actions-ci branch April 28, 2020 22:23
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.

Use GitHub actions for automated CI?
5 participants