Repository navigation
Centralize and standardize the database layer on SQLAlchemy (#3114) - #3308
dbeltrankyl wants to merge 10 commits into
Conversation
aafb9e2 to
ed42ec6
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3308 +/- ##
==========================================
- Coverage 85.28% 82.87% -2.42%
==========================================
Files 148 152 +4
Lines 22173 21555 -618
Branches 3934 3878 -56
==========================================
- Hits 18911 17863 -1048
- Misses 2501 2911 +410
- Partials 761 781 +20
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Coverage could be improved... on it |
kinow
left a comment
kinow
left a comment
There was a problem hiding this comment.
@dbeltrankyl managed to review 52 out of 76 files. Lots of changes, but quite easy as there are many that are just imports, fixing formatting, and many tests -- thanks for these improvements!
I left a few comments as I was reading it during the CES meeting 😶🌫️ 😅
Also, I think it needs to be rebased now. The docs may have a conflict soon after Suraj PR is merged.
Thanks!
| - Fix `RERUN` failing with `TypeError` on `get_job_related()` during `create` #3295 (thanks @elliot1377) | ||
| - Removed a duplicate Subversion checkout #3310 (thanks @ShivanshShukla) | ||
|
|
||
|
|
There was a problem hiding this comment.
Nit-pick, but maybe we don't need the extra blank line? 😬
| :param in_map: Whether to place the wrapper in ``job_package_map``. | ||
| :type in_map: bool | ||
| :param expected: Expected return value. | ||
| :type expected: bool |
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "dialect, expected", |
There was a problem hiding this comment.
I really don't know why, but normally pytest parameters are not separated by spaces -- https://docs.pytest.org/en/7.1.x/example/parametrize.html
| with pytest.raises(AutosubmitCritical): | ||
| db_common.check_experiment_exists(name, error_on_inexistence=error_on_inexistence) | ||
| else: | ||
| assert db_common.check_experiment_exists(name, error_on_inexistence=error_on_inexistence) is True No newline at end of file |
| pytest.param({'split': 5, 'date': '1990-01-01', 'chunk': 2}, {"splits": "{'19900101': [2, 4]}"}, True, id="dict_splits_chunk2_exceeded"), | ||
| pytest.param({'split': -1, 'date': '1990-01-01', 'chunk': 1}, {"splits": "{'19900101': [2]}"}, True, id="dict_splits_negative_split"), | ||
| pytest.param({'split': 2, 'date': '1990-01-01', 'chunk': 1}, {"splits": "{'19900101': [2]}"}, False, id="dict_splits_exact_boundary"), | ||
| ] |
There was a problem hiding this comment.
This works too, but most other tests have the cases within the @pytest.parametrized? Maybe better to follow the existing pattern for consistency.
|
|
||
| def get_log_file_path(self): | ||
| def get_log_file_path(self) -> str: | ||
| return os.path.join(self.historiclog_dir_path,f"{self.expid}_log.txt") No newline at end of file |
There was a problem hiding this comment.
Maybe add the EOL to this file too (GH UI is showing this as missing).
| """Create SQLAlchemy Core engine and resolves the connection pool class based on the backend. | ||
| # Recycle PostgreSQL connections older than this many seconds, so a connection | ||
| # silently dropped by the infrastructure is not reused. | ||
| _POSTGRES_POOL_RECYCLE_SECONDS = 1800 |
There was a problem hiding this comment.
👍 we probably need an issue to make this configurable. Doesn't need to be implemented in this PR IMo.
|
|
||
| if db_backend == "sqlite": | ||
| if db_path == ":memory:": | ||
| if str(db_path) == ":memory:": |
There was a problem hiding this comment.
I will read this file later in the IDE, but I thought this code was going to be replaced by something like _is_in_memory. But that function is only for URL.... but it's called after this, in _resolve_engine, I believe... but hard to follow that here in the GitHub UI.
| with target.begin() as conn: | ||
| conn.execute(CreateTable(table, if_not_exists=True)) | ||
| else: | ||
| target.execute(CreateTable(table, if_not_exists=True)) |
There was a problem hiding this comment.
Ah, maybe that's why you added the dbmigrate already? You replaced the old mechanism (which I think was never used, that db_version) by something that works, and that will be used by the command to migrate data? (I read your changes from the bottom-up, that's why I'm seeing this last).
Phase 7: docstring revision
bc67304 to
f7f592f
Compare
|
rebased |
2 participants
Fixes #3114
This is the big cleanup: all database access now lives under
autosubmit/database/and goes through SQLAlchemy Core. No more rawsqlite3,no more separate SQLite-only managers; the same code works on SQLite and
PostgreSQL.
It was a lot, so it's split into a 7 logical steps: ( The reviewer can compare commit by commit or just check all at once as you wish)
pragma user_version, each target database now tracks its own schema version in aschema_migrationstable.select_jobs_datais gone (we chunk theINclause instead).URL.create, define aclear pooling policy (including an in-memory
StaticPoolfor tests), andrecord schema versions with a dialect upsert so concurrent writers don't race.
let us drop the last raw-
sqlite3paths, theautosubmit.sqlschema file andthe old
db_versiontable.autosubmit/database/, split intomodels/andmanagers/.Finally some QoL extracted a shared
SchemaVersionedManager, cleaned up docstrings and type hints, and removed helpers that had no real callers anymore.One terminology note: here "schema version" means the per-target
schema_migrationstracking we added.The SQLite to PostgreSQL data migration is a different thing and is still pending (see below).
Not in this one
autosubmit dbmigrateas a placeholder.check_experiment_existsfor multi-host runs.get_experiment_descriptionshould return a plainstr.JobDataproperties once we confirm nothing external(like the Autosubmit API) depends on them.
Check List
CONTRIBUTING.md.pyproject.toml.CHANGELOG.mdif this is a change that can affect users.Closes #1234).