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

Docker vn #553

Draft
wants to merge 15 commits into
base: staging
Choose a base branch
from
Draft

Docker vn #553

wants to merge 15 commits into from

Conversation

rajatkapoordfci
Copy link

Only docker file has been added and readme file has been modified. No Code changes in VN repo otherwise.

Copy link

PRs require a priority label. Please add one.

@korikuzma korikuzma removed their assignment Apr 17, 2024
@korikuzma korikuzma self-requested a review April 17, 2024 15:14
Copy link
Member

@korikuzma korikuzma left a comment

Choose a reason for hiding this comment

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

Docker changes make sense to me. I think we just need the readme to be cleaned up a little more before merging. Please also pull changes from staging and merge into your branch

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@rajatkapoordfci
Copy link
Author

Docker_vn branch is merged with Staging branch. PR comments have been implemented.

@korikuzma korikuzma removed their assignment Apr 21, 2024
Copy link
Member

@korikuzma korikuzma left a comment

Choose a reason for hiding this comment

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

Provided some suggestions for cleaning up the readme (typos/inconsistencies)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines +8 to +12
ENV SEQREPO_ROOT_DIR=/usr/local/share/seqrepo/2021-01-29
ENV GENE_NORM_DB_URL=http://dynamodb:8001
ENV AWS_ACCESS_KEY_ID = 'DUMMYIDEXAMPLE'
ENV AWS_SECRET_ACCESS_KEY = 'DUMMYEXAMPLEKEY'
ENV AWS_DEFAULT_REGION = 'us-west-2'
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should consider having these as ARGs.

Copy link
Author

Choose a reason for hiding this comment

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

These values are required during container runtime as well. It would be better to continue with ENV unless we are planning for build time customisation.

README.md Outdated Show resolved Hide resolved
@korikuzma
Copy link
Member

@jsstevenson Please look at the above suggestions and let @rajatkapoordfci know if there are any other requested changes

rajatkapoordfci and others added 9 commits April 23, 2024 18:32
Co-authored-by: Kori Kuzma <korikuzma@gmail.com>
Co-authored-by: Kori Kuzma <korikuzma@gmail.com>
Co-authored-by: Kori Kuzma <korikuzma@gmail.com>
Co-authored-by: Kori Kuzma <korikuzma@gmail.com>
Co-authored-by: Kori Kuzma <korikuzma@gmail.com>
Co-authored-by: Kori Kuzma <korikuzma@gmail.com>
Co-authored-by: Kori Kuzma <korikuzma@gmail.com>
Co-authored-by: Kori Kuzma <korikuzma@gmail.com>
Co-authored-by: Kori Kuzma <korikuzma@gmail.com>
@jsstevenson
Copy link
Member

jsstevenson commented Apr 25, 2024

Maybe I've missed something, but I think it'd be easier to define service dependencies in a docker-compose file, rather than having the user build up everything themselves. This would obviate the need to let users define the location/port of eg the DynamoDB service since the compose file predefines everything at once.

Also -- and maybe I missed something again -- it doesn't look like there's a workflow for loading Gene Normalizer data currently (this is a requirement for the Variation Normalizer and is the reason we start a DynamoDB service).

@korikuzma
Copy link
Member

Maybe I've missed something, but I think it'd be easier to define service dependencies in a docker-compose file, rather than having the user build up everything themselves. This would obviate the need to let users define the location/port of eg the DynamoDB service since the compose file predefines everything at once.

Also -- and maybe I missed something again -- it doesn't look like there's a workflow for loading Gene Normalizer data currently (this is a requirement for the Variation Normalizer and is the reason we start a DynamoDB service).

I agree with these comments. We should probably have Docker images with pre-loaded ddb tables.

Copy link
Member

@korikuzma korikuzma left a comment

Choose a reason for hiding this comment

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

We discussed in the MetaKB meeting that this still needs work. I'm going to convert to a draft. @rajatkapoordfci please re-open and re-request our reviews once it's ready!

@korikuzma korikuzma marked this pull request as draft May 15, 2024 12:30
@jsstevenson
Copy link
Member

mentioned in the slack DM we had going -- I've been working on a similar effort in this branch, although I'm pinning specific data versions there for reasons particular to that project

korikuzma added a commit that referenced this pull request Jul 16, 2024
A working dockerfile will be added back in #553
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