[WIP] SA2.0, model/data-access edits, unit testing #17551
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jdavcs
force-pushed
the
dev_data_access
branch
4 times, most recently
from
776e6d4
to
7041b3a
Compare
jmchilton
reviewed
Feb 28, 2024
nsoranzo
reviewed
Feb 28, 2024
lib/galaxy/jobs/__init__.py
Outdated
Comment on lines
1184
to
1186
| job_id = self.sa_session.get(Job, self.job_id) | ||
| assert job_id | ||
| return job_id |
There was a problem hiding this comment.
Just realised that this is actually a job not a job_id, so wrong variable name, sorry.
There was a problem hiding this comment.
I'll squash all the commits from the previous SA20 PR into one to avoid confusion (this one has to build on that one, which is not yet merged)
jdavcs
force-pushed
the
dev_data_access
branch
6 times, most recently
from
befbbf3
to
a3bde5f
Compare
jdavcs
force-pushed
the
dev_data_access
branch
5 times, most recently
from
7ea1b84
to
d2f8e8b
Compare
jdavcs
changed the title
jdavcs
changed the title
Upgrade SQLAlchemy to 2.0 This conflicts with dependency requirements for sqlalchemy-graphene (used only in toolshed, new WIP client) Remove RemovedIn20Warning from config This does not exist in SQLAlchemy 2.0 Update import path for DeclarativeMeta Move declaration of injected attrs into constructor Remove unused import For context: https://github.com/galaxyproject/galaxy/pull/14717/files#r1486979280 Also, remove model attr type hints that conflict with SA2.0 Apply Mapped/mapped_column to model definitions Included models: galaxy, tool shed, tool shed install Column types: DateTime Integer Boolan Unicode String (Text/TEXT/TrimmedString/VARCHAR) UUID Numeric NOTE on typing of nullability: db schema != python app - Mapped[datetime] specifies correct type for the python app; - nullable=True specifies correct mapping to the db schema (that's what the CREATE TABLE sql statement will reflect). mapped_column.nullable takes precedence over typing annotation of Mapped. So, if we have: foo: Mapped[str] = mapped_column(String, nullable=True) - that means that the foo db field will allow NULL, but the python app will not allow foo = None. And vice-versa: bar: Mapped[Optional[str]] = mapped_column(String, nullable=False) - the bar db field is NOT NULL, but bar = None is OK. This might need to be applied to other column definitions, but for now this addresses specific mypy errors. Ref: https://docs.sqlalchemy.org/en/20/orm/declarative_tables.html#mapped-column-derives-the-datatype-and-nullability-from-the-mapped-annotation Add typing to JSON columns, fix related mypy errors Columns: MutableJSONType JSONType DoubleEncodedJsonType TODO: I think we need a type alias for json-typed columns: bytes understand iteration, but not access by key. Use correct type hints to define common model attrs Start applying Mapped to relationship definitions in the model Remove column declaration from HasTags parent class Fix SA2.0 error: wrap sql in text() Fix SA2.0 error: pass bind to create_all Fix SA2.0 error: use Row._mapping for keyed attribute access Ref: https://docs.sqlalchemy.org/en/20/changelog/migration_20.html#result-rows-act-like-named-tuples Fix SA2.0 error: show password in url SA 1.4: str(url) renders connection string with password SA 2.0: str(url) renders connection string WITHOUT password Solution: Use render_as_string(hide_password=False) Fix SA2.0 error: use attribute_keyed_dict Replaces attribute_mapped_collection (SA20) Fix SA2.0 error: make select stmt a subquery Rename varable to fix mypy Fix SA2.0 error: explicitly use subquery() for select-from argument Fix SA2.0 error: replase session.bind with session.get_bind() Fix SA2.0 error: joinedload does not take str args Fix use of table model attribute - Use __table__ (SA attr) instead of table (galaxy attr) on mapped classes - Drop .table and .table.c where redundant Fix bug: fix HistoryAudit model It is not a child of RepresentById becuase it does not and should not have an id attr. Duplicating the __repr__ definition in the HistoryAudit class is a temporary fix: a proper fix requires changing all models (id and __repr__ should be split into 2 mixins): to be done in a follow-up PR. Fix bug: check if template.fields is not null before iterating Fix bug: call unique() on result, not select stmt Fix bug: do not pass subquery to in_ Fix bug/typo: use select_from Fix bug: if using alias on ORM entity, use __table__ as valid FromClause Fix bug: HDAH model is not serializable (caught by mypy) Fix typing error: migrations.base Fix typing error: managers.secured This fixed 58 mypy errors! Fix typing error: session type Fix typing error: use Session instead of scoped_session No need to pass around scoped_session as arguments Fix typing error: sharable Fix SA2.0 error: sqlalchemy exceptions import; minor mypy fix Mypy: type-ignore: this is never SessionlessContext Mypy: use verbose assignment to help mypy Mypy: add assert stmt Mypy: add assert to ensure seesion is not None Calling that method when a User obj is not attached to a session should not happen. Mypy: return 0 if no results Mypy: type-ignore: scoped_session vs. install_model_session We use the disctinction for DI. Mypy: refactor to one-liner Mypy: add assert stmts where we know session returns an object Mypy: rename wfi_step > wfi_step_sq when it becomes a subquery Job search refactor: factor out build_job_subquery Job search refactor: build_stmt_for_hda Job search refactor: build_stmt_for_ldda Job search refactor: build_stmt_for_hdca Job search refactor: build_stmt_for_dce Job search refactor: rename query >> stmt Mypy: add anno for Lists; type-ignore for HDAs Note: type-ignore is due to imperative mapping of HDAs (and LDDAs). This will be removed once we map those models declaratively Mypy: managers.histories Mypy: model.deferred Mypy: arg passed to template can be None Mypy: celery tasks type ignore arg: we need to map DatasetInstance classes declaratively for that to work correctly. Mypy: type-ignore hda attr-defined error Need to map declaratively to remove this Convert visualization manager index query to SA Core Mypy: session is not none Mypy: type-ignore what requires more refactoring Mypy: type-ignore hda, ldda attrs: need declarative mapping Also, minor SA2.0 syntax fix Mypy: type-ignores to handle late evaluation of relationship arguments Mypy: type-ignore column property assignments (type is correct) Mypy: typing errors, misc. fixes Mypy: all statements are reachable Mypy: need to map hda declaratively, then its parent is model.Base Fix typing errors: sharable, secured Fix package mypy errors Fix SA2.0 error: celery task 1. In 2.0, when the statement contains "returning", the result type is ChunkedIteratorResult, which does not have the rowcount attr, becuase: 2. result.rowcount should not be used for statements containting the returning clause Ref: https://docs.sqlalchemy.org/en/20/core/connections.html#sqlalchemy.engine.CursorResult.rowcount Wrap call to ensure session is closed Otherwise there's an idle transaction left in the database (+locks) Ensure session is closed on TS Registry load Same as prev. commit: otherwise db locks are left Fix SA2.0 error: list arg to select; mypy Use NullPool for sqlite engines This restores the behavior under SQLAlchemy 1.4 (Note that we set the pool for sqlite only if it's not an in-memory db Help mypy: job is never None
Tests SQLAlchemy: 1. Create tag 2. Verify there's no FooTagAssociation with this tag 3. Add new FooTagAssociation to Foo and save to db 4. Verify there is one FooTagAssociation with this tag
No assert (only TODO); but would test SQLAlchemy
Tests SQLAlchemy, user-history relationship mapping
Tests SQLAlchemy (create foo + set foo.bar + load foo + check bar)
mypy bug workaround no longer needed as we are no longer specifying a metaclass
HasTable was a hack to accommodate declarative mapping without changing thousands of lines that referred to the `table` attribute of model instances. The one type:ignore added to managers.datasets can be removed after we map DatasetInstance models declaratively.
Uncovered by adding missing type hints to model
Drop syntax that can be inferred from Mapped[] type hint: - basic datatypes - mapped_column() expression where the only argument is the datatype Ref: https://docs.sqlalchemy.org/en/20/changelog/whatsnew_20.html#migrating-an-existing-mapping
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Simple model and data-access unit testing. Db created and loaded once per session; cleared after each test. Dependent model instances created recursively as needed, with provided arguments or reasonable defaults.
Draft.
Builds on #17180.
How to test the changes?
(Select all options that apply)
License