-
Notifications
You must be signed in to change notification settings - Fork 18
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
Propagate miniforge Docker changes #746
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, and looks like bots agree!
I did wonder if you also wanted to add the "!analyses/<module name>/<Docker & .dockerignore>"
lines to the hello-
* module run
workflow files? The only other module is metacells
which isn't using Docker yet.
- .github/workflows/run_doublet-detection.yml | ||
- "!analyses/doublet-detection/Dockerfile" | ||
- "!analyses/doublet-detection/.dockerignore" | ||
# - .github/workflows/run_doublet-detection.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had actually added this line in at one point, see here for some context: #676
If you want to keep this line deleted for now and come back to this later, I'm fine with that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I know why it is there, but it would make this PR never pass, as it would keep trying to use the current docker image, not the one defined by the changes to the Dockerfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, totally understand that, sorry if I wasn't clear! - I'm only saying if you want to wholesale delete, rather than comment out, that would be ok too.
I thought about it, but those modules don't currently actually use the Docker images, so I wasn't as worried about it, so I decided to keep the changes more limited. But I can see it going either way, really. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My one comment is let's not forget to undo the thing that is commented out.
- .github/workflows/run_doublet-detection.yml | ||
- "!analyses/doublet-detection/Dockerfile" | ||
- "!analyses/doublet-detection/.dockerignore" | ||
# - .github/workflows/run_doublet-detection.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you probably want to track reverting this in an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this as part of #676, if that works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, intended to approve!
Closes #743
This PR propagates changes from #744 to all Dockerfiles where conda is installed separately through miniforge, and to the docs with miniforge installation instructions for Docker. I had checked that #744 was successful by running the Run cell-type-ewings analysis module which is not yet complete, but is successfully running in the provided docker container.
I also temporarily removed the trigger for
doublet-detection
to run on workflow changes, just because at the moment that would run using the old docker image. Once this PR is in and the new docker image has been pushed to ECR, this change can be reverted.