Upgrade the dependency requirement of SqlAlchemy#2113
Conversation
|
Fixes #465 |
| for tag, projected_entities_dict in tag_to_projected_entity_dict.items() | ||
| } | ||
| else: | ||
| raise Exception("Got an empty dictionary") |
There was a problem hiding this comment.
Apart for the test that is failing (that I guess it's simple to fix), could you replace generic raised Exceptions with appropriate ones (here and elsewhere)? Either defined in aiida.common.exceptions, or most probably it's enough to use the standard Python ones (e.g. here I imagine a ValueError, other times it might be a TypeError or similar).
There was a problem hiding this comment.
OK for the exceptions, I put in more appropriate (b037028)
I can't reproduce the failing test locally though.
|
For the pre-commit tests that fail, I added a documentation here on how to install locally pre-commit and fix the problems: |
sphuber
changed the title
sphuber
force-pushed
the
fix_465_upgrade_sqlalchemy
branch
from
23c5750 to
979d42a
Compare
|
I fixed the problem that was failing the tests from running on Travis for SqlAlchemy. It was preventing from the pre scripts to even finish. Now the tests are running and there are still some failing tests. I also took the liberty to squash and clean up your commits. |
sphuber
force-pushed
the
fix_465_upgrade_sqlalchemy
branch
from
979d42a to
71e2132
Compare
|
The only remaining issue has to do with the |
|
I've at least isolated the problem: in while right after it is As you see I don't know why yet, and neither if this is caused by the wrapping/unwrapping done by the tests (the computer is generated by the test infrastructure) + the ModelWrapper, or it's another hidden problem. Any suggestions? |
To make the code base compatible with the new version, expressions produced by the querybuilder had to include casts from JSONB. The cast that needs to happen depends on the type of the value, but the order of the type check and the cast cannot be guaranteed in the old syntax. Therefore we replace the attribute queries from chains to cases, such that the type is checked before it is casted.
The new backend wraps the database model instances in the `ModelWrapper` utility class, which will take care of timely flushing and updating the model instance upon changes. Across the code, model instances are passed to the `flag_modified` functionn of SqlAlchemy that mark a certain key on an instance as modified. However, sometimes instead of the actual model instance being passed, the wrapped version is passed. This used to be ignored for `sqlalchemy==1.0.19` but in `sqlalchemy==1.2.12` to which we have updated, an `InvalidRequestError` is raised. To overcome this, we define our own `flag_modified` function that wraps the one of SqlAlchemy and will unwrap any wrapped instances that are passed to it.
|
I found the problem. The |
The `_set_metadata` method of `SqlaComputer` was calling `flag_modified` manually after having set the attribute, however, since the model is wrapped in the `ModelWrapper`, it's `__setattr__` will take care of calling `flag_modified` when it flushes upon an attribute being updated for a stored entity. Calling `flag_modified` on the same key twice will except in SqlAlchemy 1.2.12 because the "dirty" attribute has already been removed and moved into the database model instance.
sphuber
force-pushed
the
fix_465_upgrade_sqlalchemy
branch
from
71e2132 to
095c0d3
Compare
giovannipizzi
left a comment
giovannipizzi
left a comment
There was a problem hiding this comment.
Looks great! If tests pass, it's good to merge!! :-)
4 participants
Fixes #465
This fixes the long-standing issue of upgrading SQLAlchemy.
The way attributes stored in a JSON were queried had to be chained.
Additional tests were added to see this kind of errors before.
In essence, the issue was that the type check and value check were in arbitrary order.
Now a case statements enforces type check before value check.