search

OHDSI / Atlas

ATLAS is an open source software tool for researchers to conduct scientific analyses on standardized observational data
http://atlas-demo.ohdsi.org/
Apache License 2.0
258 stars 126 forks source link

OHDSI/Atlas#2916 - Improvements to Docker Configurability of Atlas Application #2917

Closed qcaas-nhs-sjt closed 1 month ago

qcaas-nhs-sjt commented 3 months ago

Addresses: OHDSI/Atlas#2916

chrisknoll commented 3 months ago

@alondhe , would you mind checking out these changes and provide feedback or approve?

leeevans commented 3 months ago

@qcaas-nhs-sjt thanks for your contribution!

Are you aware of the Broadsea Docker engine, docker-compose.yml file run-time container configuration approach, that OHDSI has implemented? https://github.com/OHDSI/Broadsea

There are a number of Atlas (and WebAPI) config vars in the .env file here: https://github.com/OHDSI/Broadsea/blob/main/.env

If these changes are intended to support running Atlas/WebAPI containers on K8S then it would be good to setup a quick call with @alondhe to discuss. I would also be interested to participate in that call. In the Broadsea project we don't currently have support for K8S but we would like to add it. Perhaps you could help with implementing that? For example, we probably would want to create a HELM chart and store it in the Broadsea github repo.

We should ensure that we support a single consistent set of configuration vars (env vars) regardless of the container hosting platform

If K8S support isn't the primary motivation for this PR then it would be preferable to add these configuration vars to the Broadsea .env file (if they are missing), instead of adding them to the Atlas Dockerfile.

chrisknoll commented 3 months ago

@leeevans : Thanks for your feedback. While Broadsea contains the full stack, I'm guessing that the reason that we have a docker config here in Atlas is that people may want to deploy just the Atlas app as a docker container. I think we would still want to support the userbase that has that need here, so wouldn't we want to have the same capabilities in Broadsea.

Would it make sense to develop this docker here, or would you suggest a way where you can get an atlas-only docker container out of Broadsea? I fully support the notion that the configuration params between Broadsea and here should match up.

leeevans commented 3 months ago

@chrisknoll

I believe the typical usage pattern would be to run Atlas and WebAPI in two separate Docker containers and orchestrate the two. The orchestration could be done with docker compose (as we do in Broadsea) or K8S yaml scripts.

It's possible that someone would want to just run Atlas without WebAPI, for Atlas development work but I think the focus should be on the more typical orchestrated Atlas/WebAPI combination use case.

We want Broadsea to use the docker images built from a Dockerfile in each OHDSI application github repo, like Atlas and WebAPI - so we need to keep the Dockerfile environment vars here aligned with the Broadsea .env vars approach.

If we add support for K8S (as well as continuing to support Broadsea docker compose files) then I would expect some changes to this Dockerfile will be needed and it may also support your use case of running just an Atlas Docker container.

chrisknoll commented 3 months ago

@leeevans thanks! This all makes sense to me and I appreciate that each repo would maintain their own docker info because it would be a headache for Broadsea to keep up syncing between the different dependencies.

So is the effort here to take the env config changes requested in this PR to be in agreement with env config settings that exist in broadsea?

qcaas-nhs-sjt commented 3 months ago

@qcaas-nhs-sjt thanks for your contribution!

Are you aware of the Broadsea Docker engine, docker-compose.yml file run-time container configuration approach, that OHDSI has implemented? https://github.com/OHDSI/Broadsea

There are a number of Atlas (and WebAPI) config vars in the .env file here: https://github.com/OHDSI/Broadsea/blob/main/.env

If these changes are intended to support running Atlas/WebAPI containers on K8S then it would be good to setup a quick call with @alondhe to discuss. I would also be interested to participate in that call. In the Broadsea project we don't currently have support for K8S but we would like to add it. Perhaps you could help with implementing that? For example, we probably would want to create a HELM chart and store it in the Broadsea github repo.

We should ensure that we support a single consistent set of configuration vars (env vars) regardless of the container hosting platform

If K8S support isn't the primary motivation for this PR then it would be preferable to add these configuration vars to the Broadsea .env file (if they are missing), instead of adding them to the Atlas Dockerfile.

Thanks for your comments and no, I was not aware of this implementation, I do agree that we should keep these uniform with one another. That said having reviewed the code, specifically for the atlas side of things I would strongly suggest that rather than overwriting the existing files inside of the docker image with a whole new configuration file and env substitution file, that we should instead really be looking at extending the base docker image so that it can be used in a variety of ways, else any changes we make in the base of atlas will need to be replicated in the other repository and this could eventually lead in differences between them. I would therefore suggest that we replicate the changes in Atlas and remove the injected files from broadsea.

I will review the changes in a new branch on our fork and then adjust so that they match the broadsea implementation and merge these into the PR when I've completed them and I will raise a related issue in the broadsea repository to remove the customisations from there in favour of those in the base docker image.

As for helm/k8s version of broadsea, we do have a WIP repository in our org which is public, which we could possibly adapt into implementing broadsea relatively easily. It's not perfect by any stretch and there are still a lot of issues I'm trying to work out, but it might be a good starting position:

https://github.com/lsc-sde/iac-helm-ohdsi/

Again we would want to keep these two implementations as aligned as possible, so we'd likely want to change certain things to align the solution, but I don't think that should be much of an issue.

I'm more than happy to have a call with everyone in order to discuss how we go about this moving forwards in the right way as I'd rather not keep working on this in isolation, especially as I suspect that there are problems that you have already encountered and have workarounds for where I'm still trying to figure it out.

qcaas-nhs-sjt commented 3 months ago

@leeevans @alondhe @blootsvoets @chrisknoll have refactored the environment variable names so that they should now be in-line with the broadsea implementation. Hopefully once this is in place we should be able to implement a change in broadsea to get rid of the injected configuration. Please let me know if there are any concerns.

alondhe commented 2 months ago

Apologies all - I missed this. Thanks @qcaas-nhs-sjt this is a much better approach than what we're doing in Broadsea. I think re-authoring our various Dockerfiles in repos to sync better with Broadsea while still being standalone is a great idea. I haven't tested this yet, but code looks good.