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 mutational signature dependencies to Dockerfile #295

Merged
merged 8 commits into from
Jan 7, 2023

Conversation

rjcorb
Copy link

@rjcorb rjcorb commented Dec 2, 2022

Purpose/implementation Section

What scientific question is your analysis addressing?

This PR adds dependencies for mutational signature analyses to the project Dockerfile

What was your approach?

Added new installation commands to Dockerfile for signature.tools.lib R package and libgmp3-dev

What GitHub issue does your pull request address?

Ticket #464

Directions for reviewers. Tell potential reviewers what kind of feedback you are soliciting.

Which areas should receive a particularly close look?

Confirm dependencies have been installed successfully.

Is there anything that you want to discuss further?

No

Is the analysis in a mature enough form that the resulting figure(s) and/or table(s) are ready for review?

NA

Results

What types of results are included (e.g., table, figure)?

NA

What is your summary of the results?

NA

Reproducibility Checklist

  • The dependencies required to run the code in this pull request have been added to the project Dockerfile.
  • This analysis has been added to continuous integration.

@ewafula
Copy link

ewafula commented Dec 6, 2022

@rjcorb GA checks are failing when installing sigfit package:

Error: buildx failed with: ERROR: failed to solve: process "/bin/sh -c R -e \"remotes::install_github('kgori/sigfit', ref = '209776ee1d2193ad4b682b2e2472f848bd7c67a6', build_vignettes = TRUE, build_opts = c('--no-resave-data', '--no-manual'), dependencies = TRUE)\"" did not complete successfully: exit code: 1 

@jharenza
Copy link
Member

jharenza commented Dec 6, 2022

@ewafula I was looking at this yesterday and it is an api rate limit issue, which was solved in OpenPBTA using a github PAT token - cc-ing @devbyaccident to see if we can implement here as well.

@rjcorb
Copy link
Author

rjcorb commented Dec 6, 2022

We may also be able to remove sigfit installation, since we no longer use it in the mutational-signatures module

@devbyaccident
Copy link
Member

@ewafula I was looking at this yesterday and it is an api rate limit issue, which was solved in OpenPBTA using a github PAT token - cc-ing @devbyaccident to see if we can implement here as well.

The way it looks like they did that is with a file on their build machine. I can add the PAT as a secret for the repo and expose it to the build action, I just need to generate a token. Shouldn't be a big change, especially if you already have a token I can use, maybe look for a commit with it by EoB today?

@ewafula
Copy link

ewafula commented Dec 16, 2022

@rjcorb, is this PR still needed? Associate with ticket #464

@rjcorb
Copy link
Author

rjcorb commented Dec 19, 2022

@ewafula the API rate limit issue hasn't been resolved so PR is still needed

@jharenza
Copy link
Member

To add to this - @devbyaccident won't be able to complete before the holiday, and it's possible we may not be able to remedy with GA, but can look for a solution in Jan.

@devbyaccident
Copy link
Member

Ok @jharenza @rjcorb, I added the authentication token as a build arg to the docker buildx action, and once I got the syntax right, it built and ran the job green.

I'm using the {{ secrets.GITHUB_TOKEN }} built into the repo actions context and setting it as the value for GITHUB_PAT at build time, so the install_github() function will consume it as a value for the auth_token parameter as shown in the docs.

GitHub Actions should render it as '***' in any logs, but just in case, I also changed the token's scope to read-only.

Also, I wasn't able to rebase this branch because there's just too many commits I'd need to update from main, and since it's not my code, I wouldn't be comfortable resolving all those conflicts, but I'm happy to go through re-basing commits and cleaning git history with the team at some point.

In any case, let me know if you have any questions or concerns.

@jharenza
Copy link
Member

jharenza commented Jan 7, 2023

Amazing work @devbyaccident! I'll take care of conflicts

@jharenza jharenza self-requested a review January 7, 2023 01:11
@jharenza
Copy link
Member

jharenza commented Jan 7, 2023

Going to merge this without module checks since it is only a docker image build PR.

This pull request was closed.
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.

4 participants