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.

Sign up for GitHub

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

Jump to bottom

[WIP] Disable cascade_backrefs (prep for SQLAlchemy 2.0) #13164

Closed
wants to merge 24 commits into from
Closed

[WIP] Disable cascade_backrefs (prep for SQLAlchemy 2.0) #13164

wants to merge 24 commits into from

Conversation

jdavcs
Copy link
Member

@jdavcs jdavcs commented Jan 13, 2022 •
edited
Loading

Ref #12541 (comment)

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 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 11 commits January 12, 2022 13:51
@jdavcs
Restructure HDCJSS view in model/view
67977bc
@jdavcs
Fix RemovedIn20Warning for views (see note)
effe59a 
Why we are using a separate registry and not the one containing all
models: see inline comment in parent View class)

lib/galaxy/model/view/__init__.py:70
  /home/sergey/0dev/galaxy/_galaxy/dev/lib/galaxy/model/view/__init__.py:70: RemovedIn20Warning: Calling the mapper() function directly outside of a declarative registry is deprecated. Please use the sqlalchemy.orm.registry.map_imperatively() function for a classical mapping. (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    mapper(HistoryDatasetCollectionJobStateSummary, HistoryDatasetCollectionJobStateSummary.__table__)
@jdavcs
Fix RemovedIn20Warning: use method chaining in select (see note)
99e0c5c 
lib/galaxy/model/__init__.py:8937
  /home/sergey/0dev/galaxy/_galaxy/dev/lib/galaxy/model/__init__.py:8937: RemovedIn20Warning: The legacy calling style of select() is deprecated and will be removed in SQLAlchemy 2.0.  Please use the new calling style described at select(). (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    exists([HistoryDatasetCollectionAssociation.id], and_(

lib/galaxy/model/__init__.py:8945
  /home/sergey/0dev/galaxy/_galaxy/dev/lib/galaxy/model/__init__.py:8945: RemovedIn20Warning: The legacy calling style of select() is deprecated and will be removed in SQLAlchemy 2.0.  Please use the new calling style described at select(). (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    exists([HistoryDatasetAssociation], and_(

Ref: https://docs.sqlalchemy.org/en/14/errors.html#select-construct-created-in-legacy-mode-keyword-arguments-etc
@jdavcs
Fix RemovedIn20Warning: engine.execute() (note)
daeb6cd 
test/unit/data/model/test_mapping.py::TestCleanupEvent::test_table
  /home/sergey/0dev/galaxy/_galaxy/dev/lib/galaxy/model/view/utils.py:80: RemovedIn20Warning: The Engine.execute() method is considered legacy as of the 1.x series of SQLAlchemy and will be removed in 2.0. All statement execution in SQLAlchemy 2.0 is performed by the Connection.execute() method of Connection, or in the ORM by the Session.execute() method of Session. (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    engine.execute(CreateView(view.name, view.__view__))

https://docs.sqlalchemy.org/en/14/changelog/migration_20.html#execute-method-more-strict-execution-options-are-more-prominent
@jdavcs
Fix RemoveIn20Warning: do not autocommit (note)
0dc8e3f 
test/unit/data/model/test_mapping.py: 12 warnings
  /home/sergey/0dev/galaxy/_galaxy/dev/lib/galaxy/model/migrate/versions/util.py:204: RemovedIn20Warning: The current statement is being autocommitted using implicit autocommit, which will be removed in SQLAlchemy 2.0. Use the .begin() method of Engine or Connection in order to use an explicit transaction for DML and DDL statements. (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    connection.execute(cmd)
@jdavcs
Fix RemoveIn20 warning: use engine.begin for explicit transaction
ec455b0
@jdavcs
Fix RemoveIn20Warning: use test() to wrap raw sql
b613c74
@jdavcs
Fix RemovedIn20Warning: engine.execute
74cc259
@jdavcs
Fix RemoveIn20Warning: Query.join(str)
df6cf53 
RemovedIn20Warning: Using strings to indicate relationship names in Query.join() is deprecated and will be removed in SQLAlchemy 2.0.  Please use the class-bound attribute directly.
@jdavcs
Disable cascade_backrefs: ToolShedRepository/ToolDependency
d634e31
@jdavcs
Disable cascade_backrefs: ToolShedRepository/RRDAssoc
cafefdd
@jdavcs jdavcs added this to the 22.05 milestone Jan 13, 2022
@jdavcs jdavcs added area/database Galaxy's database or data access layer kind/refactoring cleanup or refactoring of existing code, no functional changes status/WIP and removed area/testing labels Jan 13, 2022
mvdbeek
mvdbeek reviewed Jan 14, 2022
View reviewed changes
Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

I'm afraid that for a lot of these we don't have great test coverage (that also asserts whatever modification was made actually got persisted). Can we add a config flag that turns on

When 2.0 deprecation warnings are enabled,

so we can collect some data on this on usegalaxy.org ?

@mvdbeek mvdbeek modified the milestones: 22.05, 22.01 Jan 14, 2022
@jdavcs
Copy link
Member Author

jdavcs commented Jan 14, 2022

I'm afraid that for a lot of these we don't have great test coverage (that also asserts whatever modification was made actually got persisted). Can we add a config flag that turns on...

I think that's a good idea. Then we could use the following sequence for addressing this:

  1. turn on warning (via config setting)
  2. address each warning that suggests a future runtime error (I can think of 2 at this point: cascade-backrefs removal and session autocommit removed) - i.e., add a test exposing it, fix, etc...
  3. add cascase_backrefs=False (this PR) to all relevant relationships, address any new issues. Make any other changes (session commit handling, etc.), resolve new issues.

Because, now that I think of it, we don't want to silence the warnings before fixing the underlying cause. Obviously...
The warnings should be helpful: at least the ones I've seen that were triggered by tests point to specific lines.

  /home/sergey/0dev/galaxy/_galaxy/dev/test/unit/data/model/mapping/test_install_model_mapping.py:135: RemovedIn20Warning: "RepositoryRepositoryDependencyAssociation" object is being merged into a Session along the backref cascade path for relationship "ToolShedRepository.required_repositories"; in SQLAlchemy 2.0, this reverse cascade will not take place.  Set cascade_backrefs to False in either the relationship() or backref() function for the 2.0 behavior; or to set globally for the whole Session, set the future=True flag (Background on this error at: https://sqlalche.me/e/14/s9r1) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    obj.repository = repository

@jdavcs jdavcs mentioned this pull request Jan 14, 2022
Merged
3 tasks
@jdavcs
Copy link
Member Author

jdavcs commented Jan 14, 2022

Closing this, since we've decided not to silence the warnings, at least not until we've addressed each cause.

@jdavcs jdavcs closed this Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvdbeek mvdbeek mvdbeek left review comments

Assignees
No one assigned
Labels
area/database Galaxy's database or data access layer kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Milestone
22.05
Development

Successfully merging this pull request may close these issues.
2 participants
@jdavcs @mvdbeek