-
Notifications
You must be signed in to change notification settings - Fork 12
Add Postgres support to Autosubmit #2187
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
Conversation
85b14a4 to
430758d
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2187 +/- ##
==========================================
+ Coverage 63.73% 65.55% +1.81%
==========================================
Files 82 84 +2
Lines 19332 19697 +365
Branches 3758 3814 +56
==========================================
+ Hits 12321 12912 +591
+ Misses 6077 5840 -237
- Partials 934 945 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
430758d to
304853f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
304853f to
a282ac2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Fixed one test, then the integration test run command started failing locally, so I started reviewing what was going on and merging the |
35f091b to
5275f2b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
7f031d6 to
66f05e1
Compare
|
The tests were passing locally when executed individually. I think @LuiggiTenorioK mentioned some time ago that there was a possibility of some race condition between the two PG tests we have at the moment. So I bit the bullet and implemented part of #2250 here, using TestContainers' Postgres context-manager in a fixture, launching a container with Postgres for each test, using a random port, updating the connection string URL, and running in parallel. The tests in test_db_common are a bit slow, so after fixing the pipeline I'll investigate if we can speed that up in #2250 . |
fe8c689 to
85a35f5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Bug found when trying workflow with vertical wrapper 🐛 : It is in the autosubmit/autosubmit/autosubmit.py Lines 2173 to 2176 in 1a2fcf4
|
|
Thanks @kinow ! Following this comment: I won't be there in person on Monday, so I would prefer to perform the merge into the master on Tuesday to also check the rebase of my branch (I foresee issues mainly with the test configuration-related changes). |
Ah, first 🐛 of this review 🙂 I'll check it on Monday. Thanks @LuiggiTenorioK ! |
Sounds good to me, @dbeltrankyl ! |
|
Checking for similar cases like the one mentioned in #2187 (comment) The same will happen in autosubmit/autosubmit/autosubmit.py Lines 5478 to 5485 in 1a2fcf4
autosubmit/autosubmit/autosubmit.py Lines 2828 to 2834 in 1a2fcf4
|
|
Added as autosubmit/4.1.16-dev-postgres-1a2fcf4 into the climate-dt ( VERSION file points to 4.1.15, tho) |
|
I see this warning /appl/AS/4.1.16-dev-postgres-1a2fcf4/lib64/python3.9/site-packages/networkx/utils/backends.py:135: RuntimeWarning: networkx backend defined more than once: nx-loopback Aside from it, the sqlite database run seems to work in the climate-dt |
|
Huh, hopefully that warning isn't anything serious. I've fixed the issues reported by @LuiggiTenorioK . Luiggi was right about the |
|
From #2627 (comment), I added the details table to be created when using autosubmit install. |
|
I will rebase and resolve the conflicts later today. I think they might be coming from the ruff merge we did recently (@VindeeR we'll see if the linter is working!) |
Thanks!! |
8166c62 to
b1518bc
Compare
|
Linter failed, then I executed this on my machine |
|
Additional issue: the |
|
I'm adding the |
59232fe to
1f5c059
Compare
Add Postgres support to Autosubmit Implemented with SQLAlchemy, abstract/OOP/protocols, a new configuration key DATABASE_BACKEND. Tested with Docker and TestContainers. More tests added (unit and integration).
Closes #1285
Supersedes #1914
PR to enable Postgres support in Autosubmit.
The database was not normalized. We moved the tables as close to what they were as possible, to minimize the risk of bugs. For Sqlite everything behaves the same as before. For Postgres, we adopted the approach of one Postgres DB Schema per experiment.
Note, too, that this change does not move the Pickle file. That file (the job list) will remain on disk for now, but in a future change will be moved to the DB but in a simplified manner (i.e. we will not dump the whole object as a blob, it will be probably a structured graph for the job list).
Tasks
db_structuredatabase/db_common.pyget_experiment_id_save_experiment(maybe not needed if only called from sqlite functions... it uses sqlite exception check, checking)_check_experiment_exists(ditto)_update_experiment_descrip_version(ditto)_get_autosubmit_version(ditto)_last_name_used(ditto)_delete_experiment(ditto)_update_database(ditto)_get_experiment_id(ditto)database/db_manager.py(select_all_wherehad been deleted, probably clean-up with vulture)database/db_structure.pyexperiment/detail_updater.pyexperiment/experiment_common.pyexperiment/experiment_common.pyhistory/database_managers/database_manager.pyhistory/database_managers/database_models.pyhistory/database_managers/experiment_history_db_manager.pyhistory/database_managers/experiment_status_db_manager.pymigrate/migrate.py(it's raising an exception, and it's not supposed to work in PG for now)provenance/rocrate.py(check what will happen with thepklfiles, if these are replaced)autosubmit configureManual tests
As this change touches important parts of the system, we are performing a few additional manual tests, despite the unit and integration tests already added.
To test locally, the following Docker container is used. Note that it intentionally uses arguments different from those used in tests and from the default values. That's to validate that we do not have anything hardcoded that just works by accident.
$ docker run --rm \ -e POSTGRES_PASSWORD=lavanda \ -e POSTGRES_USER=sound \ -e POSTGRES_DB=testingsomething \ -p 5432:5432 \ postgresMy~/.autosubmitrc:[database] backend = postgres connection_url = postgresql://sound:lavanda@localhost:5432/testingsomething path = /home/bdepaula/autosubmit filename = autosubmit.db [local] path = /home/bdepaula/autosubmit [globallogs] path = /home/bdepaula/autosubmit/logs [structures] path = /home/bdepaula/autosubmit/metadata/structures [historicdb] path = /home/bdepaula/autosubmit/metadata/data [historiclog] path = /home/bdepaula/autosubmit/metadata/logs [autosubmitapi] url = http://192.168.11.91:8081 # Replace me?Note
@LuiggiTenorioK updated the
configuresubcommand, and now one can simply run (thanks BTW!)That should give you the same
.autosubmitrcas above, but with less typing :)Every test is being performed with two experiments, so that we can verify database integrity. One test is performed, and then the database values are dumped. Then the next test is performed, and another database dump is done. The dumps are diffed, and the database values are compared with the database client.
These are the test workflows being used: A dummy workflow, the auto-mhm workflow from RO-Crate, a few examples from our documentation, the ClimateDT workflow.
The following operations are performed for every workflow during the tests. Creation (expid), create the graph (create), refresh the experiment project sources (refresh), run the workflow (run), recover jobs (recovery), monitoring (monitor), commands generation (inspect), variable generation (report), stats report (stats), set status of tasks (setstatus), stop workflow (stop), and deletion (delete).
expid is tested twice, for the first workflow and then a second time for its copy (i.e. we are testing both
expidandexpid -y).Dummy is a local project, but auto-mhm and ClimateDT are Git projects. ClimateDT uses Git hooks.
The deletion of experiments is important because
autosubmit.pyhas logic to deletejob_data.sql, and we need to verify what will happen when using Postgres. One experiment, the auto-mhm, contains RO-Crate configuration, so that will validate the provenance gathering in the workflow too.Further testing will be performed in EDITO later.
After @dbeltrankyl 's joblist -> DB change, we need to test:
autosubmit createputs the job data into the correct tablesautosubmit deleteremoves it ; the current code is leavingjob_pklpolluted, but that table is expected to be deleted when Dani's work is merged. Still, we need to test this.See @LuiggiTenorioK 's note on how to build the container configured for Postgres: #2406 (comment)