Sqlalchemy compositional

[nickeopti]

Implement the compositional TerracottaDriver structure proposed in #240 (comment).

In a separate PR (going into #240) for better diffs.

[nickeopti]

I really like this structure. The concept of lifting out functionality to the TerracottaDriver is really nice! This has made the code in RelationalDriver (in particular) and RasterDriver a bit leaner.

However, I have encountered two problems, which I need help to fix.
First is CI related; the tests fail in Initialize mypy.
And when we fix that, we will see another problem; something pickle related regarding futures in https://github.com/DHI-GRAS/terracotta/blob/sqlalchemy-compositional/terracotta/drivers/raster_base.py#L523 fails, which in turn makes a lot of stuff fail. I hope this is an easy fix, but I have no clue where to start

[dionhaefner]

Thanks for the implementation. I'll take care of the CI failures.

[codecov]

Codecov Report

Merging #248 (b2ebcba) into sqlalchemy (da2bc5b) will increase coverage by 0.56%.
The diff coverage is 98.42%.

@@              Coverage Diff               @@
##           sqlalchemy     #248      +/-   ##
==============================================
+ Coverage       97.98%   98.54%   +0.56%     
==============================================
  Files              46       48       +2     
  Lines            2080     2135      +55     
  Branches          294      299       +5     
==============================================
+ Hits             2038     2104      +66     
+ Misses             24       19       -5     
+ Partials           18       12       -6     

Impacted Files	Coverage Δ
terracotta/drivers/geotiff_raster_store.py	95.18% <95.18%> (ø)
terracotta/raster.py	98.28% <98.28%> (ø)
terracotta/drivers/__init__.py	100.00% <100.00%> (ø)
terracotta/drivers/base_classes.py	100.00% <100.00%> (ø)
terracotta/drivers/mysql_meta_store.py	100.00% <100.00%> (ø)
terracotta/drivers/relational_meta_store.py	98.90% <100.00%> (ø)
terracotta/drivers/sqlite_meta_store.py	100.00% <100.00%> (ø)
terracotta/drivers/sqlite_remote_meta_store.py	100.00% <100.00%> (ø)
terracotta/drivers/terracotta_driver.py	100.00% <100.00%> (ø)
terracotta/scripts/optimize_rasters.py	95.94% <100.00%> (ø)
... and 2 more

---

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 da2bc5b...b2ebcba. Read the comment docs.

[dionhaefner]

Is this ready for review @nickeopti ?

[nickeopti]

Yes, I believe so!

Documentation is probably lacking a bit, but I think the code is ready 😄

I'm debating whether the standardize_keys in driver.py is best as a decorator, or simply as a function that gets called. Also, the requires_connection decorators in driver.py are solely there for proper error-handling and roll-backs -- not because the methods actually needs any connection. It works, but may seem slightly weird.

Thanks a lot for the fixes!

[dionhaefner]

I'll review this soon, then. In the meantime, could you please bump the test coverage a bit? In general we don't accept PRs that lower global coverage (without a good reason).

[nickeopti]

I've bumped test coverage a bit. There are now two uncovered branches left in this PR's diff -- neither of which I know how to hit.
The coverage also slightly decreases in #240, but I believe that is because the number of total lines of code decreases -- the PR's diff itself has 100% coverage.

[dionhaefner]

Thanks, this looks very nice!

Some minor issues but overall this looks like a huge improvement of the code structure.

[nickeopti]

Yep, the pytest-flake8 and pytest 7 combination seems to be the culprit: tholo/pytest-flake8#83. This problem applies to the entire project. Will you pin the version?

[mrpgraae]

Yep, the pytest-flake8 and pytest 7 combination seems to be the culprit: tholo/pytest-flake8#83. This problem applies to the entire project. Will you pin the version?

👌
Pinned now.

[dionhaefner]

The pytest-flake8 issue is fixed in #252 and will land in main soon.

[dionhaefner]

Some last nitpicks.

[dionhaefner]

More issues:

• Docs are out of date (but I can take care of them after everything else is stable)
• I would like to revisit whether RemoteSQLite should inherit from SQLite. I would argue that it should not inherit, because it increases complexity and saves you a handful lines of code at best, but if both of you prefer inheritance I'm OK with it.

[mrpgraae]

More issues:

* [ ]  Docs are out of date (but I can take care of them after everything else is stable)

* [ ]  I would like to revisit whether `RemoteSQLite` should inherit from `SQLite`. I would argue that it should not inherit, because it increases complexity and saves you a handful lines of code at best, but if both of you prefer inheritance I'm OK with it.

Docs are out of date (but I can take care of them after everything else is stable)

I guess as long as they get updated before the next release, it should be fine. Maybe we can build them as part of the CI chain?

I would like to revisit whether RemoteSQLite should inherit from SQLite. I would argue that it should not inherit, because it increases complexity and saves you a handful lines of code at best, but if both of you prefer inheritance I'm OK with it.

This must be a very subjective thing, because in my mind it's less complex in the current structure 😄 This way, I can look at RemoteSQLite and see it as a "diff" of how it behaves differently from the regular SQLite backend. If we did not have inheritance, there would be duplicate code and I would have to cross-reference them to understand the difference.

[dionhaefner]

Great job everyone - a real team effort.