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

Maintainance/docker dev env #32

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jspaezp
Copy link
Contributor

@jspaezp jspaezp commented Nov 29, 2022

Adds docker container for usage in development.

Why?

  1. Since there are dependencies in the package that are not automatically resolved using conda in M1 macs, an alternative is to use a container for development, where all local test runs occur within the container.

Specification for dev containers:
https://containers.dev/overview

Copy link
Member

@wfondrie wfondrie left a comment

Choose a reason for hiding this comment

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

I think this is a good addition, but we need to document how to use it, perhaps in the README.

As a side note, I can successfully build the production image by specifying the platform:

docker build --platform linux/amd64 .

Comment on lines +4 to +6
"customizations": {
"vscode": {}
}
Copy link
Member

Choose a reason for hiding this comment

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

question: What does this customization do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a placeholder for vscode plugins .... can be removed
Nontheless, the whole impact of this PR is reduced by #35

@@ -0,0 +1,8 @@

FROM --platform=linux/amd64 ghcr.io/talusbio/nf-encyclopedia:latest
RUN apt update && apt install default-jdk-headless git docker -y
Copy link
Member

Choose a reason for hiding this comment

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

question: Have you tried used the version of the JRE that is installed in the production image?

We build on top of the official EncyclopeDIA image, which uses the openjdk:8-jre as their base image. I don't know if the JRE version will work with NextFlow, but it might be worth checking.

Copy link
Member

Choose a reason for hiding this comment

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

@wfondrie
Copy link
Member

Is this PR still needed with #35?

@jspaezp
Copy link
Contributor Author

jspaezp commented Nov 29, 2022

I really like the concept of this pr but ... since right now I am experiencing a weird issue where the tests occasionally stall. Which I believe could be somehow related to the way that nextflow detects that a spawned process has finished, which might be weird when running docker inside groovy inside python inside docker .... So it think we can leave this in the back burner for a while and just document the patch for MSSTATS on mac.

@jspaezp
Copy link
Contributor Author

jspaezp commented Nov 29, 2022

@wfondrie

Is this PR still needed with #35?

It still has a place, but i dont think its worth the effort right now, although i do believe we need to document how to patch the msstats situation in m1 macs ... (because I will forget immediately ...)

@jspaezp jspaezp marked this pull request as draft December 15, 2022 00:09
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

2 participants