-
Notifications
You must be signed in to change notification settings - Fork 34
Pytest xdist with parallel postgres docker containers #860
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
base: develop
Are you sure you want to change the base?
Conversation
ac8d96b to
a5c7ffc
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #860 +/- ##
===========================================
- Coverage 84.52% 84.17% -0.35%
===========================================
Files 35 35
Lines 4245 4278 +33
Branches 536 537 +1
===========================================
+ Hits 3588 3601 +13
- Misses 462 481 +19
- Partials 195 196 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| - name: Install Dependencies | ||
| run: | | ||
| uv sync |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is missing --locked --extra=test.
| - name: Run Tests | ||
| run: | | ||
| uv run pytest -v -x integration_tests/ --numprocesses 4 --dist load --durations 20 --junit-xml pytest_junit.xml --cov=--cov-report=xml -r sx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Github's Linux runners have 2 cores, is 4 a good number for these tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that has changed, but a pleasant surprise!
Put $(nproc) instead of 4 then, so it will automatically adjust with the number of cores in the runners in the future.
|
|
||
|
|
||
| GET_DB_FROM_ENV = "get-the-db-from-the-environment-variable" | ||
| POSTGIS_IMAGE = "postgis/postgis:16-3.5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit afraid of having many places to update for the same piece of information because they tend to get out of sync, can this be read from docker-compose.yaml instead so things can't go out of sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
I'll find somewhere better for this, an environment variable, or a pytest argument, or reading from a config, or something.
|
Having 4 CI jobs for testing was my workaround for the tests taking 30 minutes to run, and often failing intermittently by the closed cursor. You have fixed the intermittent failures though, and it would be much better to use one job that runs things in parallel. Starting up a bunch of database containers seems like a quite heavy-handed way that might consume quite a lot of memory. Would it be difficult to instead have a single database container and run the tests against multiple databases inside that container instead? |
| "Initialising ODC Schema for testing", odc_db=odc_db, cfg_db_url=cfg_env.db_url | ||
| ) | ||
| index = index_connect(cfg_env, validate_connection=False) | ||
| # With permissions causes trouble when run multiple times on the same database |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what problem you're seeing, but could you be hitting opendatacube/datacube-core#1959?
| with conn.cursor() as cur: | ||
| log.info("Creating database", db_name=test_database_name) | ||
| cur.execute( | ||
| sql.SQL("CREATE DATABASE {db_name}").format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a dinner break since my earlier comments and I still love the thought of faster testing, but this feels like a big chunk of dynamic stuff that will be really hard to inspect/reproduce when things go mysteriously wrong in the CI.
We can easily create $(nproc) test databases when we spin up the current database container (just add a for loop loop around the create database part of the create_db.sh script in commit e6e823b), isn't there some place in conftest.py (or similar) where we can patch in the worker number into the database-part of the engine connection string?
As context, my primary motivation was to run the tests faster locally, not just in GHA. It would be really nice if we can use the same approach everywhere.
That would be nicer, I tried, but, it was unexpectedly difficult with the way I thought I could solve it by getting the first test worker, named |

WIP
This PR fixes the old support for spinning up a bunch of postgis docker container for testing against in parallel. It ended up broken when the new postgis 1.9 driver was integrated.
It allows running tests on any machine with docker, by running
uv run pytestand to use pytest-xdist to run the tests significantly faster by splitting them across multiple processes by specifying--numprocesses auto(or replaceautowith a number).The
cubedash.testutils.databasemodule provides fixtures to:xdist--numprocesses)xdisthas a couple of styles of splitting, which both work for me.--dist loadscopesplits the tests up while running all tests from the same test module on the same test worker.--dist loadignore modules and splits the tests more evenly across test workers.The fixtures and usage of
cubedash.testutils.databaseare such that either option works. On my 2024 MacBook Pro (12 ARM cores),loadscopecompletes the full test suite in about 5 minutes, andloadin about 5 minutes. Howeverloadcauses way more load and churn, since duplicate test databases will be setup multiple times for the same test module on different workers.When running with
xdist--pdbflag is disabled.📚 Documentation preview 📚: https://datacube-explorer--860.org.readthedocs.build/en/860/