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 template file for GHA #11

Merged
merged 3 commits into from
Oct 1, 2020
Merged

Conversation

LiNk-NY
Copy link
Contributor

@LiNk-NY LiNk-NY commented Sep 11, 2020

@lcolladotor
This will create a working check-bioc.yml in the .github/workflows directory based on the
user's Bioconductor version and R version.
Windows and Mac tests are not supported yet.

@LiNk-NY
Copy link
Contributor Author

LiNk-NY commented Sep 24, 2020

Hi Leo, @lcolladotor
This should add multiple platform support. Let me know what you think.
Best,
Marcel

@FelixErnst
Copy link

Hi Marcel @LiNk-NY, Hi Leonardo @lcolladotor,

I setup GHA for new project and I came across the issue, that I need GHA to work with devel on master and with release on another branch due to changes in S4Vectors (the renaming of vertical_slot_names to parallel_slot_names).

Wouldn't it make sense, to suggest the GHA to be active by default for a branch != master if the version is release? Bioconductor expects development to happen on master, so maybe this warrants a little bit of nudging towards avoiding a potential problem. Maybe this can be solved with an interactive choice.

Let me know, what you think. Thanks.

Felix

@LiNk-NY
Copy link
Contributor Author

LiNk-NY commented Sep 25, 2020

Hi Felix, @FelixErnst
This should work on both release and devel. I will update the documentation soon to provide more guidance.
If you're on Bioc-release, you'd run this the same way and push to your RELEASE_3_11 branch.
You'd have two 'check-bioc.yaml' files, one on the release branch and another in the master branch.

@FelixErnst
Copy link

Hi Marcel,

I didn't want to suggest the GHA not working 😄 . I am sorry if that's how it came across.

The suggestion is mostly about the first 3 lines in the GHA template file, which don't restrict the GHA to a certain branch by default.

For example, if devel is specified as version this wouldn't make too much sense:

on:
  push:
    - RELEASE_3_11
  pull_request:
    - RELEASE_3_11

Vice verse if release is specified, this is also problematic in some cases within half a year:

on:
  push:
    - master
  pull_request:
    - master

Also leaving the on argument empty works at the beginning, but within a release cycle, this may change.

use_bioc_github_action currently doesn't take any arguments, but maybe an optional branch argument could help populate the restrictions and make the user aware of bioconductor expecting the master branch to work with devel.

@LiNk-NY
Copy link
Contributor Author

LiNk-NY commented Sep 25, 2020

Hi Felix, @FelixErnst

I didn't want to suggest the GHA not working . I am sorry if that's how it came across.

No worries, I didn't interpret it that way. I was trying to say that it is designed to work based on the user's current Bioconductor installation. That is, if you are running Bioc-devel, the file that is generated would be for devel.

The suggestion is mostly about the first 3 lines in the GHA template file, which don't restrict the GHA to a certain branch by default.

For example, if devel is specified as version this wouldn't make too much sense:

on:
  push:
    - RELEASE_3_11
  pull_request:
    - RELEASE_3_11

Vice verse if release is specified, this is also problematic in some cases within half a year:

on:
  push:
    - master
  pull_request:
    - master

AFAICR, this is not necessary when you have a yaml file in each branch. I could be wrong though.
I haven't tested this given that the builds were broken yesterday.

Also leaving the on argument empty works at the beginning, but within a release cycle, this may change.

The code is dependent on BiocManager so it should work for every release cycle.

use_bioc_github_action currently doesn't take any arguments, but maybe an optional branch argument could help populate the restrictions and make the user aware of bioconductor expecting the master branch to work with devel.

See point above. The normal workflow is to have the master branch point to the Bioc devel branch. Note that the user can always edit their file after it is generated by the function.

@FelixErnst
Copy link

FelixErnst commented Sep 26, 2020

Ok maybe using it will tell us, how it behaves during two release cylces.

The code is dependent on BiocManager so it should work for every release cycle.

My guess is (As you said we couldn't test it yesterday), that once a package has a RELEASE branch, the GHA config will be applied as defined for all branches. At that point a new devel version is available, which is not necessarily compatible with the on before the RELEASE branch was created,

In that scenario, using bioc-devel as defined in the GHA leads to potential check errors on the RELEASE branches, if the user does not intervene, because devel before RELEASE is fine and devel after RELEASE is not fine per se. The time point when the GHA is run makes the difference.

See point above. The normal workflow is to have the master branch point to the Bioc devel branch. Note that the user can always edit their file after it is generated by the function.

Of course, but adding a GHA for the master branch only, will lead to expected behavior on the master branch and will solve future issues on RELEASE branches by not being run on those.

So I would add at the limit to the master branch in GHA template by default. Otherwise users might "complain" after half a year. With the limit they might "complain" as well, but not because of an error, but because they need help to apply GHA to a different branch.

But hey, this is really a minor issue. Thanks for adding the template!

@LiNk-NY
Copy link
Contributor Author

LiNk-NY commented Sep 27, 2020

Hi Felix! @FelixErnst

My guess is (As you said we couldn't test it yesterday), that once a package has a RELEASE branch, the GHA config will be applied as defined for all branches. At that point a new devel version is available, which is not necessarily compatible with the on before the RELEASE branch was created,

Have a look at this release branch package with a GitHub Action: https://github.com/waldronlab/cBioPortalData/blob/RELEASE_3_11/.github/workflows/main.yml
A GHA takes precedence if present on that branch. It only gets run when pushes are made to the RELEASE_3_11 branch.
When a maintainer creates a release branch on GitHub, they should run the use_bioc_github_action function on that branch.
Other branches are expected to be development branches so I think it would be advantageous to have the checks run the GHA for other branches.

In that scenario, using bioc-devel as defined in the GHA leads to potential check errors on the RELEASE branches, if the user does not intervene, because devel before RELEASE is fine and devel after RELEASE is not fine per se. The time point when the GHA is run makes the difference.

There is no version mix up because the GHA will be using the appropriate Docker container or BiocManager command. As long as containers are up to date, that should not be a problem.

@lcolladotor
Copy link
Owner

Wow, this is a really fancy PR!

@LiNk-NY
Copy link
Contributor Author

LiNk-NY commented Oct 1, 2020

Thanks Leo! I'm trying to do the package justice 😉

@lcolladotor
Copy link
Owner

Hi! This is an older discussion on the GHA forum that was partly why I ended up having the code duplication for docker then mac & windows https://github.community/t/run-matrix-job-on-macos-and-on-ubuntu-in-container/16359.

Having said that, the template could have common pieces of code and re-use the code that is duplicated.

lcolladotor added a commit to lcolladotor/testmatrix that referenced this pull request Oct 1, 2020
@lcolladotor
Copy link
Owner

lcolladotor commented Oct 1, 2020

I've been working on this for a while, but well, I'm a stuck. I got your version to work, then started adding the features that biocthis has. And it nearly works. The issue I'm having is that I can't get GHA to let me specify volumes: /home/runner/work/_temp/Library:/usr/local/lib/R/host-site-library without this crashing the Windows and macOS runs. If I don't use that, then I can't restore the R packages cache on Linux running bioconductor's docker image (that's the status at lcolladotor/testmatrix@10484be).

@lcolladotor
Copy link
Owner

At lcolladotor/testmatrix@285cfbe (aka https://github.com/lcolladotor/testmatrix/blob/285cfbe4dc277fd71e9de5ada77b2eed8dff65a1/.github/workflows/check-bioc.yml) I got the cache to work with bioc-docker and my test was successful ^^ https://github.com/lcolladotor/testmatrix/runs/1192083901?check_suite_focus=true#step:9:22

I'll merge your PR tomorrow then =)

I'm still thinking about the part of setting the versions vs having the GHA workflow detect them from the git branch when you make a git push. Your approach is likely better though it involves a few more commits (every time bioc-devel changes, to update the R version) which I know is actually just once per year. I guess that also invites users to download the latest GHA workflow on biocthis and at least compare against it, in case it has new features, etc.

Anyway, this will be a major change since it reduces nearly all the code duplication (the R cache part is duplicated right now, though that can be solved with the config matrix if the temp directory path is consistent on Windows; that's where the R user library lives; I'll check more logs tomorrow).

Thanks again!

@lcolladotor lcolladotor merged commit 55dfbc1 into lcolladotor:master Oct 1, 2020
lcolladotor added a commit that referenced this pull request Oct 1, 2020
I added a note about the change in functionality at actions/check-bioc.yml
lcolladotor added a commit that referenced this pull request Oct 1, 2020
lcolladotor added a commit to LieberInstitute/megadepth that referenced this pull request Oct 1, 2020
lcolladotor added a commit to LieberInstitute/recount3 that referenced this pull request Oct 1, 2020
lcolladotor added a commit to LieberInstitute/megadepth that referenced this pull request Oct 1, 2020
lcolladotor added a commit to LieberInstitute/spatialLIBD that referenced this pull request Oct 1, 2020
lcolladotor added a commit that referenced this pull request Oct 1, 2020
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.

None yet

3 participants