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

Replace SQLAlchemy Migrate with Alembic #13513

Merged
merged 54 commits into from
Mar 11, 2022

Conversation

jdavcs
Copy link
Member

@jdavcs jdavcs commented Mar 9, 2022

This supersedes #13108. Please check that PR for a description of the new migration system and its testing infrastructure, how to test manually, and discussion.

Why a new branch/PR
Merging #13461 introduced conflicts in config/__init__.py and model/mapping.py that were not easy to resolve. The gist of it is that both PRs moved and refactored parts of these two files early on, with multiple commits following in either case. As a result, the alembic branch has 50+ commits building upon code that does not exist anymore. After trying several approaches, I've settled on the following:

The following will be submitted as a follow-up PR immediately after this is merged (these edits do not belong in this PR):

How to test the changes?

(Select all options that apply)

License

  • I agree to license these contributions under Galaxy's current license.
  • I agree to allow the Galaxy committers to license these and all my past contributions to the core galaxy codebase under the MIT license. If this condition is an issue, uncheck and just let us know why with an e-mail to [email protected].

jdavcs added 21 commits March 9, 2022 18:40
Otherwise alembic/env.py gets executed out of context at test collection
stage (via tox -e unit >> run_tests.sh), which raises an AttributeError
(alembic.context is not set in this case)
Otherwise alembic/env.py gets executed out of context at test collection
stage (via tox -e unit >> run_tests.sh), which raises an AttributeError
(alembic.context is not set in this case)
To eliminate the repetitive `engine_options = engine_options or {}` in
multiple places.
This tests sqlalchemy migrate which is being replaced.
(does not include an import in mapping.py: to be added in a separate
commit, grouped with other edits in mapping.py)
Squashed:
- pick 01a42712d2 Avoid extra trip to db via lazy load of alembic version heads
- squash 8d729974d6 Fix mypy error
- pick 058f09a Integrate migrations design into config and model code
- squash 89ca1a537b Drop config.database_create_tables (not used)

Next squash:
- pick cbf29e138c Integrate migrations into config, model (squashed)
- squash dd51fd578e Address existing mypy error (see note)

Address existing mypy error (see note)

This is a (possible) bug that has existed on dev. To expose it, simply
change this call to init_models_from_config
(https://github.com/galaxyproject/galaxy/blob/release_21.09/lib/galaxy/config/__init__.py#L1302)
to a call directly to mapping.init(). The result will be 6 mypy errors.

The reason is that the mapping.init() function
(https://github.com/galaxyproject/galaxy/blob/release_21.09/lib/galaxy/model/mapping.py#L79)
specifies its return type as GalaxyModelMapping - which results in
several errors of this type:

lib/galaxy/app.py:178: error: Definition of "model" in base class "MinimalApp" is incompatible with definition in base class "ConfiguresGalaxyMixin"

When we call an intermediary function (that does not have a return type
specified), mypy is happy.

This should be addressed in a separate issue/PR. Removing the return
type is the simplest (temporary) solution.

Next squash:
- squash c842973 Add type hints to migrations modules
- squash a54be22 Remove circular dependency between migrations and config

Setup infrastructure for running db scripts (squashed)

Squashed:
- pick 146ff8019a Setup infrastructure for running db scripts
- squash e0a26dcf3a Move is-one-db check into model.database_utils

Squashed:
- pick a82c44a Setup infrastructure for running db scripts (squashed)
- squash 31a1a702fe Fix mypy error

Squashed:
- pick 14a4eb08e2 Setup infrastructure for running db scripts (squashed)
- squash b1c6acb0f1 When overriding, override.

Add type hints to migrations modules

Remove circular dependency between migrations and config
Also add exec permissions to create/migrate python scripts.
(not sure this is required, adding for consistency with previous
version)

+ A few fixes for TS scripts, tests

Squashed:
- pick b624432 Add create/migrate scripts for Alembic dbs
- squash 06f0a92 Fix integration test (test only relevant for TS)
@jdavcs jdavcs added this to the 22.05 milestone Mar 10, 2022
@jdavcs jdavcs changed the title [WIP] Reorganized alembic branch, prep for rebase Reorganized alembic branch Mar 10, 2022
@jdavcs jdavcs marked this pull request as ready for review March 10, 2022 17:59
@jdavcs jdavcs force-pushed the dev_alembic20_rebase2_1 branch from c8ee55e to 55448d4 Compare March 11, 2022 05:41
See inline comment ("version too old" is misleading if the incorrect
version is more recent than what the system expects).
@jdavcs jdavcs force-pushed the dev_alembic20_rebase2_1 branch from 55448d4 to 44d245f Compare March 11, 2022 05:53
@mvdbeek mvdbeek merged commit 63ab546 into galaxyproject:dev Mar 11, 2022
@mvdbeek
Copy link
Member

mvdbeek commented Mar 11, 2022

Thanks a lot @ic4f, very nice work!

@bgruening
Copy link
Member

Incredible work, thanks @ic4f!

@jdavcs
Copy link
Member Author

jdavcs commented Mar 11, 2022

Thank you for merging, @mvdbeek! And thank you, everyone, for reviewing these branches! 🎆

@mvdbeek mvdbeek changed the title Reorganized alembic branch Replace SQLAlchemy Migrate with Alembic Jun 28, 2022
@jdavcs jdavcs added the highlight Included in user-facing release notes at the top label Jul 6, 2022
@hexylena hexylena added highlight/dev Included in admin/dev release notes and removed highlight Included in user-facing release notes at the top labels Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/configuration Galaxy's configuration system area/database Galaxy's database or data access layer area/documentation area/scripts area/testing highlight/dev Included in admin/dev release notes kind/feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants