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

Update docker-compose #386

Merged
merged 18 commits into from
Nov 21, 2024
Merged

Update docker-compose #386

merged 18 commits into from
Nov 21, 2024

Conversation

PGijsbers
Copy link
Collaborator

@PGijsbers PGijsbers commented Nov 6, 2024

Change

Update the docker compose file to:

  • avoid mounting source directories by default, which poses a risk that deployed versions may run code that is not predefined in the image (e.g., local hotfixes instead of through releases)
  • provide a secondary compose file which may be used to mount all the source code instead
  • define the PYTHONPATH in the image always, since it's needed to function if there is no mounted source.
  • remove the version top level element which is obsolete

Between the override.env introduced in #383, the develop compose file, and the different profiles, running common use case commands becomes very verbose (e.g., docker compose --env-file=.env --env-file=override.env -f docker-compose.yaml -f docker-compose.dev.yaml --profile openml --profile huggingface-datasets up -d). I introduced two scripts to provide shorthands:

  • scripts/up.sh: takes as arguments any number of profile names and will automatically specify the override environment, as well as the dev compose file if AIOD_MOUNT environment variable is set.
  • scripts/down.sh: brings down all services (i.e., it uses --profiles "*")

How to Test

These changes fall outside the scope of unit tests, as they only touch auxiliary scripts and docker.

Since Dockerfiles are modified, images need to be rebuilt:

docker compose --profile "*" build
./scripts/clean.sh

then start the containers, optionally with profiles, e.g.,:

./scripts/up.sh openml huggingface-datasets zenodo-datasets examples

This should bring up all containers, honoring the overrides in override.env but without mounting your local version of the source code.
Modify the local source code, e.g., leave a comment in src/main.py and verify it isn't present in a running container

docker exec -it apiserver /bin/bash
cat main.py

To test the containers with mounted source code, simply set AIOD_MOUNT=1 in override.env. Then re-up the containers, e.g.:

./scripts/down.sh
./scripts/up.sh examples

When you check the container's content, you should see it now reflects your local changes.

Checklist

  • Tests have been added or updated to reflect the changes, or their absence is explicitly explained.
  • Documentation has been added or updated to reflect the changes, or their absence is explicitly explained.
  • A self-review has been conducted checking:
    • No unintended changes have been committed.
    • The changes in isolation seem reasonable.
    • Anything that may be odd or unintuitive is provided with a GitHub comment explaining it (but consider if this should not be a code comment or in the documentation instead).
  • All CI checks pass before pinging a reviewer, or provide an explanation if they do not.

Related Issues

@PGijsbers PGijsbers marked this pull request as ready for review November 6, 2024 15:09
@PGijsbers PGijsbers added the enhancement New feature or request label Nov 6, 2024
.env Show resolved Hide resolved
@PGijsbers PGijsbers requested review from andrejridzik and removed request for andrejridzik November 6, 2024 15:16
@PGijsbers
Copy link
Collaborator Author

Delaying review request because I realized the wrong configuration file is mounted.

When the file does not exist on the host machine, docker-compose
will create the "file" as a directory when mounting. This is
harmless so long as we handle that case correctly.
Copy link
Collaborator

@andrejridzik andrejridzik left a comment

Choose a reason for hiding this comment

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

Looks good to me and very useful! 🙂
The only confusing part to me is the name AIOD_MOUNT and the way of defining the value (anything vs something). However, it is well described in the documentation and only used for development purposes, so I don't see it as a big problem.

@PGijsbers
Copy link
Collaborator Author

I don't mind changing the name. I just wanted it to indicate that 1) it mounts the source code and 2) it's AIOD specific. Probably AIOD_MOUNT_SOURCE would've been better? Or any other suggestion?

@andrejridzik
Copy link
Collaborator

andrejridzik commented Nov 12, 2024

I am not exactly sure what you mean by AIoD specific. Perhaps I would emphasize the development aspect, so something like AIOD_DEV_MOUNT, but AIOD_MOUNT_SOURCE is also fine. On the other hand, the variable is actually used only in the script up.sh and does not indicate any mount, but rather we just decide whether we use docker-compose.dev.yaml or not. If we want to keep it extendable in the future, we should just name it something like USE_LOCAL_DEV or so.
The other part of my comment was to perhaps consider using the values 0/1 or true/false, as it is not clear what "is set" actually means (even though it is mentioned in the comment). It would make it more clear if one needs to explicitly mention value true in order to avoid mistakes (setting it to false will deploy it as dev currently).

@PGijsbers
Copy link
Collaborator Author

Good remarks. Will update tomorrow.

Also require set to "true" rather than any value, to avoid
accidental usage.
@PGijsbers
Copy link
Collaborator Author

@andrejridzik I renamed the environment variable and moved the --reload use to the dev configuration. Additionally, I also moved some keycloak configurations.

I think that in the future we should fix the non-dev docker-compose to use published images only (e.g., metadata-catalogue:v1.3.20241011) and specify the build directives only in local, too. But I think this is orthogonal and perfectly suitable as a separate PR. I propose we merge this now.

@PGijsbers
Copy link
Collaborator Author

I'll go ahead and merge this. Feel free to leave your thoughts, if there's work resulting from it, we can address it in a follow-up PR.

@PGijsbers PGijsbers merged commit 5eefa4d into develop Nov 21, 2024
4 checks passed
@PGijsbers PGijsbers deleted the docker/update-compose branch November 21, 2024 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants