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

SQLAlchemy 2.0 follow-up #19388

Merged
merged 4 commits into from
Jan 10, 2025
Merged

SQLAlchemy 2.0 follow-up #19388

merged 4 commits into from
Jan 10, 2025

Conversation

jdavcs
Copy link
Member

@jdavcs jdavcs commented Jan 8, 2025

  1. Remove cascade_backrefs=False:
    No longer needed after move to SQLAlchemy 2.0
    https://docs.sqlalchemy.org/en/20/changelog/migration_14.html#cascade-backrefs-behavior-deprecated-for-removal-in-2-0
  2. Disable transaction helper.
    The helper checks if a transaction is active and begins one if not. There are 500+ instances where this helper is called. I'd like to first check that disabling it does not break transactional state; if this does not cause new session-related errors, I'll remove the function and the calls to it.
    Ref: Towards SQLAlchemy 2.0: drop session autocommit setting #15421

If this does not break transactional state, this helper should beremoved completely.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

jdavcs added 2 commits January 7, 2025 19:12
If this does not break transactional state, this helper should be
removed completely.
@jdavcs jdavcs added kind/refactoring cleanup or refactoring of existing code, no functional changes area/database Galaxy's database or data access layer labels Jan 8, 2025
@jdavcs jdavcs added this to the 25.0 milestone Jan 8, 2025
@jdavcs jdavcs requested a review from a team January 8, 2025 21:27
Copy link
Member

@nsoranzo nsoranzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the plan to remove the transaction helper as part of this PR if tests pass, or in a follow-up PR after some real-world testing?

lib/galaxy/model/base.py Outdated Show resolved Hide resolved
lib/galaxy/model/base.py Outdated Show resolved Hide resolved
@nsoranzo nsoranzo merged commit 9953e07 into galaxyproject:dev Jan 10, 2025
53 of 56 checks passed
@jdavcs
Copy link
Member Author

jdavcs commented Jan 10, 2025

Is the plan to remove the transaction helper as part of this PR if tests pass, or in a follow-up PR after some real-world testing?

@nsoranzo A follow-up PR, but very soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/database Galaxy's database or data access layer kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants