pacificclimate / modelmeta

An ORM representation of the model metadata database
GNU General Public License v3.0
1 stars 0 forks source link

Model discrete sampling geometry datasets #62

Closed rod-glover closed 6 years ago

rod-glover commented 6 years ago

Resolves #60

  1. Includes Alembic schema+data migration. Both upgrade and downgrade have been tested on a SQLite db but not a Postgres db.

  2. Modifies indexer and tests to work with new ORM/schema, but only for gridded datasets, which were the kind handled originally. Indexing DSG TS datasets will be the subject of another PR.

  3. Cleans up and PEP8-ifies v2 ORM code, which was a little untidy.

  4. Adds tests specifically for new, polymorphic ORM for discrete sampling and gridded sampling geometries. These may seem a little superfluous, but they are a way of ensuring that we correctly understand how SQLAlchemy handles polymorphism/inheritance. Turns out it's pretty straightforward.

TODO:

rod-glover commented 6 years ago
  1. Yes, sorry about mixing in the PEP-8 fixup. I can remove it if you like and put it in a separate PR. Shouldn't be very difficult. A product of Friday-night bleariness.

  2. So far, migrations don't have unit tests. The reason is that within a PR, the ORM matches the final migrated state of the database, not an earlier state. The unit tests (relying on testing.postgresql) just create the database from the (current) ORM. There are a couple of ways we could create a unit test environment for the upgrade migrations:

    1. Stash an old version of the ORM, create a test database based on that, populate it with some test data, migrate, run unit tests.
    2. Populate and dump a Postgres database in the old state, load it up in, migrate, run unit tests.
    3. Both of those solutions have always seemed fairly effortful to me, and so I rely on testing against local databases, whether SQLite or Postgres. I am currently making a copy of ce_meta locally so that I have something safe to test against.
  3. For the downgrade migrations, it's easier, since you're downgrading from a latest version of the ORM.

rod-glover commented 6 years ago

OK, embarassing, there's this. I'll look into it and any similar packages.

jameshiebert commented 6 years ago

OK, embarassing, there's this. I'll look into it and any similar packages.

Wouldn't call that embarrassing. I didn't know about that, and it looks like a good resource. To boot, I hadn't thought through the fact that to test the database, your initial instance will be the current ORM schema. In any case, checking out alembic-verify sounds like a good path.

I can remove it if you like and put [the PEP8 migration] in a separate PR.

No don't worry about it. Just keep it in mind for next time.

jameshiebert commented 6 years ago

LGTM. Are you happy with this at present @rod-glover ?

rod-glover commented 6 years ago

@jameshiebert : One small addition: Migration now makesdata_files.x_dim_name, .y_dim_name nullable so that we don't have to fill in nonsense values for them for DSG data files. We didn't really make a decision whether or not to remove these (I'd like to), so this seemed the prudent choice at the moment. We can migrate those columns out later if we decide to. I'll put in an issue about that.

I am happy with this now and ready to merge if you are.