aiidateam / disk-objectstore

An implementation of an efficient "object store" (actually, a key-value store) writing files on disk and not requiring a running server
https://disk-objectstore.readthedocs.io
MIT License
15 stars 8 forks source link

Lift partially limit on SQLAlchemy version #108

Closed giovannipizzi closed 3 years ago

giovannipizzi commented 3 years ago

Limit was on sqlalchemy<1.4. I think this was because of python 3.5 support being dropped in SQLAlchemy 1.4. Now that #106 is merged (py3.5 support dropped), I'm here moving the limit to <2, so we check this works.

giovannipizzi commented 3 years ago

I didn't check this yet - let's see what errors we get, if any

sphuber commented 3 years ago

I think this was because of python 3.5 support being dropped in SQLAlchemy 1.4. Now that #106 is merged (py3.5 support dropped), I'm here moving the limit to <2, so we check this works.

It couldn't have been this because sqlalchemy specifies the python_requires in its setup.py and so pip will not install an incompatible version. As you can see the tests do actually fail, seemingly because of a problem with sqlalchemy except I have to admit I don't really understand what it refers to.

giovannipizzi commented 3 years ago

Ok thanks for the message. I'll moved this back to draft, so we can use the branch to test how to fix the problem.

giovannipizzi commented 3 years ago

For my own reference: https://docs.sqlalchemy.org/en/14/changelog/migration_14.html I think the issue might be related to the change with the old way of bulk update/delete. It used to be

session.query(User).filter(User.name == 'sandy').update({"password": "foobar"}, synchronize_session="fetch")

and will be

with Session(engine, future=True) as sess:
    stmt = update(User).where(
        User.name == 'sandy'
    ).values(password="foobar").execution_options(
        synchronize_session="fetch"
    )

    sess.execute(stmt)

I guess it should be easy to fix, but I will need to investigate more to check if this is really the thing to fix. For now, I leave this as a note, not to forget.

giovannipizzi commented 3 years ago

I fixed the issue, but I wouldn't merge this yet - I think it makes sense to replace all session.query() with the 2.0 syntax so we're future proof. In any case, this is not urgent, as long as also AiiDA does not support SQLA 1.4 we won't use the SQLA 1.4-compatible version of this library.

codecov[bot] commented 3 years ago

Codecov Report

Merging #108 (b0218d4) into develop (7a894a4) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop      #108   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines         1541      1544    +3     
=========================================
+ Hits          1541      1544    +3     
Impacted Files Coverage Δ
disk_objectstore/container.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7a894a4...b0218d4. Read the comment docs.

chrisjsewell commented 3 years ago

superceded by #114