Enables PIXL DB to operate as a service separate from Orthanc Raw

[tomaroberts]

Description

Fixes #517:

Adds the means to create two postgres services. Previously, PIXL DB was used by both Orthanc Raw and for storing patient metadata.

Now it is possible to have separate databases for Orthanc Raw and PIXL DB. By default and in keeping with previous design, the root ./docker-compose.yml brings up a single postgres service, shared by both Orthanc Raw and PIXL DB.

With this PR, test/docker-compose.yml now contains a new postgres service demonstrating how to bring up a separate service for PIXL DB.

Type of change

Please delete options accordingly to the description.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Suggested Checklist

• I have performed a self-review of my own code.
• I have made corresponding changes to the documentation.
• My changes generate no new warnings.
• I have commented my code, particularly in hard-to-understand areas.
• I have passed on my local host device. (see further details at the CONTRIBUTING document)
• Make sure your branch is up-to-date with main branch. See CONTRIBUTING for a general example to syncronise your branch with the main branch.
• I have requested review to this PR.
• I have addressed and marked as resolved all the review comments in my PR.
• Finally, I have selected squash and merge

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.64%. Comparing base (28db049) to head (5dfad17).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #593      +/-   ##
==========================================
+ Coverage   87.61%   87.64%   +0.02%     
==========================================
  Files          76       76              
  Lines        3505     3512       +7     
==========================================
+ Hits         3071     3078       +7     
  Misses        434      434              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[p-j-smith]

looking very nice!

I think the docs could do with updating in a couple of places:

• https://github.com/SAFEHR-data/PIXL/tree/main/cli#configuration
• https://github.com/SAFEHR-data/PIXL/tree/main/orthanc/orthanc-raw#step-1

And possibly some more places too. Which makes me wonder whether it would be worth describing all the environment variables in a single README rather than across the repo (as a separate PR)?

Also, if we use an external postgres instance, would we need to change how we handle alembic migrations? Currently we run them when the container is started, but I guess we'd need to apply them manually if postgres is running elsewhere?

[stefpiatek]

Looking good, thanks for getting this in 🎉 . Agree with Paul on the documentation places. Happy for you to merge once you've added that in (without another review) and resolved the other comments to your liking.

Currently we run them when the container is started, but I guess we'd need to apply them manually if postgres is running elsewhere?

I think its fine for us to still run migrations on imaging api going up, because it'll still need to check the migrations have happened anyway.

[tomaroberts]

A note: my last commit removed the env_file: line from test/docker-compose.yml and now the system test is passing. But that's slightly unexpected?

Two thoughts:

1. I think the original problem was due to relative file paths. See here for a similar issue around relative paths and env-file:. I think the context: attribute is only applied to build: rather than the location of the file associated with env-file: attribute.
2. Reading the DC docs, there is some level of inheritance of env variables across compose files – this might explain the tests passing now because a) the env variables already exist and b) by removing the lines in 502f27e we remedy the broken file pathing.

[stefpiatek]

1. think the original problem was due to relative file paths. See here for a similar issue around relative paths and env-file:. I think the context: attribute is only applied to build: rather than the location of the file associated with env-file: attribute.

Yeah I think that's right - IIRC we have to do the test docker compose up --build in two steps because of this

[tomaroberts]

@stefpiatek – are you happy if we just remove the offending lines from the test/docker-compose.yml then? As per 502f27e

[tomaroberts]

@stefpiatek

@p-j-smith and I have discussed at (surprising) length how best to name the environment variables as it's fiddly and quite confusing. This simultaneously feels like making a mountain out of a molehill, so please excuse the lengthy comment, but also we're mindful of causing confusion further down the line for someone who might want to setup an external PIXL DB.

We think that having the variable names POSTGRES_ORTHANC_RAW_DB_PORT and POSTGRES_PIXL_DB_PORT is optimal (albeit imperfect). With these variables, we are trying to differentiate between the postgres dbs associated with Orthanc Raw and PIXL DB.

We figured that your suggestions of using:

• EXTERNAL_PIXL_DB_POST doesn't quite meaningfully fit with where it is called inside _config.py because it might not be external
• Similarly calling it CLI_PIXL_DB_PORT doesn't feel explicit enough that the variable is about postgres and also for consistency, this would mean renaming POSTGRES_ORTHANC_RAW_DB_PORT > ORTHANC_RAW_DB_PORT, which is already something else 😱

So...

I've added a comment to the .env.sample to hopefully make it clear that the ports should both be 7001 for a purely local deployment and then POSTGRES_PIXL_DB_PORT can become 7011 if it is required as an external db.

[tomaroberts]

@stefpiatek @p-j-smith

I'm going to tentatively say this is done now 🥶, if you guys wouldn't mind taking a look!

For Stef, the docker-compose now has two db services:

• postgres-exposed: used by default when running pixl dc up. This is the 'old' way of doing things where the db is shared by Orthanc Raw and PIXL DB and a single service is used to both connect to Orthanc and expose the PIXL DB port.
  • Note: if the user runs pixl dc up, under the hood, by default it runs pixl dc up --profile postgres-exposed. We figured this was the easiest and cleanest way of implementing rather than forcing users to always run pixl dc up --profile blah
• postgres: can be invoked by running pixl dc up --profile postgres. The postgres service connects to Orthanc Raw via the Docker network and then, as seen in /test/docker-compose.yml, the postgres-pixl-db service exposes the CLI_PIXL_DB_PORT env variable; to whatever the external PIXL DB is located on.

Paul – some of the tests were failing after our last discussion. The main one was around _parse_up_args where the order of the args for the pixl dc up command were important. The "up" arg needed to come after "--profile postgres-exposed":

PIXL/cli/src/pixl_cli/_docker_commands.py

Lines 44 to 54 in d05829d

	def _parse_up_args(args: tuple[str, ...]) -> list:
	"""Check up args and define default profile if not set"""
	args_list = list(args)
	if "--profile" not in args:
	args_list = [arg for arg in args_list if arg != "up"] + [
	"--profile",
	"postgres-exposed",
	"up",
	]
	args_list.extend(["--wait", "--build", "--remove-orphans"])
	return args_list

(There were also a bunch of typing issues which mypy was angry about, hence changing code more explicitly to lists)

[p-j-smith]

Looking great! Nice job getting this working. Just a few small suggestions

[stefpiatek]

Looks really nice, one request to have the profile work from config and not from having to remember to call the right profile. Once you've done that I'm happy for you to merge 😄

[tomaroberts]

I will merge tomorrow, unless someone spots something in the meantime.

Thanks for your help both, especially Paul – owe you a 🍺 / 🍸 / ☕ (delete as preferred).

[p-j-smith]

nice work @tomaroberts !

Thanks for your help both, especially Paul – owe you a 🍺 / 🍸 / ☕ (delete as preferred).

Ooo I do like me a nice ☕