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 docker-compose-dev #554

Closed
wants to merge 7 commits into from

Conversation

chhsiao1981
Copy link
Contributor

add docker-compose-dev

@jbernal0019
Copy link
Member

@chhsiao1981 I think this should go in a new separate repo.

@jennydaman
Copy link
Contributor

@jbernal0019 how come? The added files for docker-compose are for the purpose of CUBE development, with the CUBE source code mounted into the container.

I think this should be merged. The next steps would be to move the imperative invocation of steps e.g. unit tests and integration tests into the docker-compose YAML too, so that unit tests and integration tests can be invoked simply by calling docker compose run test instead of ./make.sh

@chhsiao1981
Copy link
Contributor Author

@jbernal0019

Given that this PR is extremely helpful in ChRIS_ultron_backEnd development / seeing how ChRIS_ultron_backEnd really works, I strongly recommend that this PR be merged.

cc: @jennydaman

@chhsiao1981 chhsiao1981 force-pushed the docker-compose-dev branch from cbff2ba to a6f8a18 Compare May 7, 2024 15:04
- pip install -r /opt/ChRIS_ultron_backEnd/requirements/local.txt && python manage.py migrate --noinput
volumes:
- chris_files:/var/chris:rw
- ./chris_backend:/opt/app-root/src
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason we need to mount /opt/app-root/src and /opt/ChRIS_ultron_backEnd?

Also, should we use :ro on the source file mounts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback.

  1. /opt/app-root/src is for the entrypoint, and /opt/ChRIS_ultron_backEnd is to easily know what we are running.
  2. :ro is good to have, but the goal of this docker-compose-dev.yml is for development. I don't think :ro is necessary though~


chris:
container_name: chris
image: ghcr.io/fnndsc/cube:5.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Since many of these services specify pip install -r ... in their command, I would suggest using a docker-compose build.

image: ghcr.io/fnndsc/chris_ui:latest-staging
command: sirv --host --single
environment:
REACT_APP_CHRIS_UI_URL: http://localhost:8000/api/v1/
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the environment variables should be VITE_APP_CHRIS_UI_URL and VITE_APP_PFDCM_URL

image: ghcr.io/fnndsc/pman:6.0.1
container_name: pman
environment:
CONTAINER_ENV: docker
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this is a breaking change.

pman needs to create containers using Docker. There are two available APIs for this:

  • Docker API: simpler code and usage
  • Swarm API: more complicated

There are different features available depending on whether the Docker API or Swarm API is used. For example, the Docker API supports configurations CONTAINER_USER and SHM_SIZE, but does not support GPU. The Swarm API does not support CONTAINER_USER nor SHM_SIZE but Swarm API does work with GPU.

The discrepancy in features is not a technical limitation but rather just laziness.

While I strongly suggest using CONTAINER_ENV=docker, @jbernal0019, who is the main contributor of ChRIS_ultron_backEnd, is against CONTIANER_ENV=docker and favors CONTAINER_ENV=swarm.

More context: https://github.com/fnndsc/pman?tab=readme-ov-file#swarm-vs-docker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback.

I believe that the goal for docker-compose-dev.yml is for development only, not for production.
having CONTAINER_ENV=docker should be enough for quick development.

Copy link
Contributor

@jennydaman jennydaman May 10, 2024

Choose a reason for hiding this comment

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

pman was updated in FNNDSC/pman@5b6f202 to support GPUs with CONTAINER_ENV=docker

@chhsiao1981 chhsiao1981 force-pushed the docker-compose-dev branch from 1616e56 to ccf595c Compare May 7, 2024 20:19
@chhsiao1981 chhsiao1981 force-pushed the docker-compose-dev branch from ccf595c to e463a76 Compare May 7, 2024 23:30
@jennydaman
Copy link
Contributor

Closing in favor of #566

@jennydaman jennydaman closed this Aug 29, 2024
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