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 rOpenScPCA to renv/docker in ewing module #881

Conversation

allyhawkins
Copy link
Member

Purpose/implementation Section

Please link to the GitHub issue that this pull request addresses.

Towards #878

What is the goal of this pull request?

Here I'm just adding rOpenScPCA to the lock file for renv and subsequently the Docker image. I also noticed a typo in the GHA for building the ewing's docker image that I fixed.

Briefly describe the general approach you took to achieve this goal.

Installed the package and then took a snapshot.

If known, do you anticipate filing additional pull requests to complete this analysis module?

Yes!
Next up, I'll be updating the cluster template to use the functions in rOpenScPCA. I also plan on incorporating Leiden clustering results to the plots now that we have that functionality in rOpenScPCA.

Author checklists

Analysis module and review

Reproducibility checklist

  • Code in this pull request has been added to the GitHub Action workflow that runs this module.
  • The dependencies required to run the code in this pull request have been added to the analysis module Dockerfile.
  • If applicable, the dependencies required to run the code in this pull request have been added to the analysis module conda environment.yml file.
  • If applicable, R package dependencies required to run the code in this pull request have been added to the analysis module renv.lock file.

@allyhawkins allyhawkins requested review from sjspielman and removed request for jaclyn-taroni November 14, 2024 20:01
Copy link
Member

@sjspielman sjspielman left a comment

Choose a reason for hiding this comment

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

👍

@@ -49,7 +49,7 @@ jobs:
- name: Build image
uses: docker/build-push-action@v6
with:
context: "{{defaultContext}}:analyses/simulate-sce"
context: "{{defaultContext}}:analyses/cell-type-ewings"
Copy link
Member

Choose a reason for hiding this comment

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

seems like a good typo to fix!

@sjspielman
Copy link
Member

Actually @allyhawkins how did you add rOpenScPCA? Did you snapshot, and if so, is there a file w/ library(rOpenScPCA) here?

@allyhawkins
Copy link
Member Author

Actually @allyhawkins how did you add rOpenScPCA? Did you snapshot, and if so, is there a file w/ library(rOpenScPCA) here?

I did use renv::snapshot(). But I'm thinking that snapshot worked because I started using the package in the template notebook to prepare for the next PR/ to make sure it was working. I can add it to the dependencies.R file temporarily until that PR goes in.

@sjspielman
Copy link
Member

But I'm thinking that snapshot worked because I started using the package in the template notebook to prepare for the next PR/ to make sure it was working. I can add it to the dependencies.R file temporarily until that PR goes in.

Makes sense, I don't think you need to add anything here. I have a lot of confidence it will be used soon!

@@ -40,7 +40,7 @@ jobs:
test-build:
name: Test Build Docker Image
if: github.event_name == 'pull_request' || (contains(github.event_name, 'workflow_') && !inputs.push-ecr)
runs-on: ubuntu-latest
runs-on: openscpca-22.04-big-disk
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain to me why we need this? It will be challenging to manage this under our current paradigm.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I was getting an error running out of disk space when trying to build the image (see https://github.com/AlexsLemonade/OpenScPCA-analysis/actions/runs/11844604285).

I can see if I can make the image smaller?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or skip the test build for this module... but that seems not great.

Copy link
Member

Choose a reason for hiding this comment

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

I've made a change for now, but if we could make this smaller, that'd be swell. I prefer how we were handling permissions.

@allyhawkins allyhawkins merged commit ad8b1dc into AlexsLemonade:main Nov 15, 2024
5 checks passed
@allyhawkins allyhawkins deleted the allyhawkins/ewings-add-ropenscpca-to-docker branch November 15, 2024 18:04
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.

3 participants