Add tilegarden #610

KlaasH · 2018-11-30T22:03:13Z

Overview

Copies over enough of Tilegarden to get a tiler service running in development and serving census blocks, ways, and bike infrastructure tiles (though not filtered by job).

It doesn't copy over any of the configuration code (i.e. .env.template and the terraform config), since the plan is to integrate at least some of that into the existing deployment infrastructure for this project. So it's development-only at the moment. There are a few remnants (e.g. deploy and related commands in the project.json file) that are in what was copied but won't currently work.

Still to do:

Add filtering by job (Add filtering by job ID to Tilegarden endpoint #609)
Modify the tile URLs returned by the API (Enable using non-S3 tile endpoints #592)
Get it deploying (i.e. adapt and integrate Tilegarden's deployment process into the scripts and config for this project) (Set up remote development Tilegarden endpoint using Staging database #593, Configure CloudFront proxy for tile endpoint #594, Deploy Tilegarden tiler to staging #595)

Demo

Notes

See the commit messages for some additional notes and details.

Testing Instructions

If you haven't run the migration from PR #600, run that first. If you don't have any completed analysis jobs at all, you'll need to run one to have some data to look at.

Run scripts/setup or (in the VM) scripts/update to build, then scripts/server
Go to the front-end app (http://localhost:9301), click through to a neighborhood, and show one of the layers listed under "Overlays" in the layer picker.
Open up the debug console and pick the "Network" tab, then move or zoom the map to get it to request some new tiles.
Copy the URL of one of the tile requests from your development S3 tile source, then replace everything up to the first number (i.e. the Z value in the tile path) with http://localhost:9400/tile/, e.g. http://localhost:9400/tile/14/4770/6200.png.
That request will fail, telling you you need to supply a config parameter. Add ?config=ways, ?config=census_blocks, or ?config=bike_infrastructure to see some actual tiles.

Also, scripts/test should run Tilegarden's tests in addition to the Django tests, and should pass.

Resolves #591.

CloudNiner

Mostly looks good, I just have a few questions added within the commit log since it was easier to parse the changes one commit at a time. I like the removal of babel for simplicity, this now looks more like node code. Worked after provision with no issues, tested on our favorite Germantown boundary.

There's quite a lot changed already in here wrt upstream tilegarden. Are we keeping a log of those changes somewhere for future reference?

So far I have:

import -> require
promisify inline -> dependency
remove utf/vector tile code
Switch to Azavea fork of claudia-local-api
Change browserify port
Change DB env variable names

CloudNiner · 2018-12-03T16:25:43Z

src/tilegarden/package.json

@@ -29,7 +29,7 @@
    "destroy": "claudia destroy",
    "dev": "nodemon -e js,mss,json,mml,mss --ignore bin/ --ignore '*.temp.mml' --exec yarn local",
    "lint": "eslint src",
-    "local": "yarn transpile && node --inspect=0.0.0.0:9229 -- node_modules/claudia-local-api/bin/claudia-local-api --abbrev 300 --api-module bin/api | bunyan -o short",
+    "local": "yarn transpile && node --inspect=0.0.0.0:9401 -- node_modules/claudia-local-api/bin/claudia-local-api --abbrev 300 --api-module bin/api | bunyan -o short",


To avoid unnecessary changes to the tilegarden diff, might want to just leave this one as is and make the docker port mapping 9401:9229.

CloudNiner · 2018-12-03T16:27:12Z

src/tilegarden/src/config/map-config.mml

-    dbname: ${TILEGARDEN_DB}
-    user: ${TILEGARDEN_USER}
-    password: ${TILEGARDEN_PASSWORD}
+    host: ${PFB_DB_HOST}


This is a far reaching change that impacts upstream changes when we can just map the environment variable in the docker compose config. Are we sure we want to do this now?

The env munging change seems separate, and ok.

The config files where this is used are already project-specific. I think adding generality to how configuration is assembled (especially not having to repeat the whole database config for every layer) is probably something that should happen in Tilegarden, but as it stands right now changing these variables doesn't amount to changing anything very general. So I think the benefit of not having two copies of these variables to define and keep in sync in PFB is enough that it's worth switching.

Makes sense.

KlaasH · 2018-12-03T19:37:40Z

There's quite a lot changed already in here wrt upstream tilegarden. Are we keeping a log of those changes somewhere for future reference?

Good point. I just made azavea/tilegarden#129 as a place to keep an annotated list of what changes we're making and what they mean for future development on Tilegarden proper.

I also switched the inspect port back (I was going to rebase it in, but the port switch came before the removal of transpilation, and it didn't seem worth the trouble to resolve the conflicts).

CloudNiner

LGTM, feel free to merge pending tests passing. Looks like a missing eslint configuration in the tilegarden source directory?

Copies the parts of Tilegarden that will be needed here into this project, pruning a bit (but not extensively--there may still be some cruft) and changing identifiers in package.json but not making any substantive changes.

Adds the tilegarden container to the docker-compose config. It wasn't immediately obvious how to change claudia-local-api to use a different port, so I just mapped the default port inside the container to a chosen one outside. Also splits the "common" docker-compose config, because I wanted to inherit the environment variables but it seemed too weird for this node container to inherit a "django" config.

Since this uses a Node 8.10 runtime, most newer EMCAScript features should be supported without transpilation. It appears that the only thing missing that we're using is the import/export syntax. So this refactors our imports to use `require` and `module.exports` and removes all the babel pieces. Since we're not using a 'bin/' directory anymore, this also changes the config compilation command to put the xml back in the source directory alongside the mml and mss. And adds the resulting files to .gitignore. This also adds `ncc`, which will hopefully work for making an efficient deployment bundle, but that part is not actually tested. Putting it in as a placeholder seemed better than either leaving the babel commands in place or ripping out all the bundling-related pieces then having to add them back later. Add compiled config files to .gitignore

Especially since we're only actually using one of the functions in one place, just using `promisify` in tiler.js is simple and works fine.

Switches the map-config.mml to use the PFB database variables. Since we will likely be pursuing a different environment-management strategy from the "variable name prefix" one, this drops the part where it prefixes the environment to the variable name before looking it up.

Matt DelSordo's fork has some fixes that are needed for Tilegarden to work properly. This switches to a fork of that that we can continue developing on.

Get ready for gigantic diffs for tiny changes!

When running as a development server, it can be useful to get a shell.

Removes the default map-config.mml/style.mss config and adds separate configs for the three PFB tile layers. The styles were copied from the ones in src/tilemaker/working_files and modified only to switch field names to lowercase (since the field names in the shapefiles were all caps but in the database they're all lower).

An unqualified tile URL will produce a somewhat unhelpful message if there's no default config file. This catches errors from opening the config file and sets a more helpful message in that situation.

We're not going to be using utfGrids or vector tiles here, so there's no reason to keep the code around in this repo (we could always copy it back in if we change our mind).

Doesn't remove the old 'tilemaker' yet, since it hasn't been replaced yet elsewhere in the code.

Adds the Tilegarden test command to `scripts/test` and gets the tests running by removing a few tests of removed code and fixing up the imports for the shift to non-transpiled code.

KlaasH · 2018-12-03T20:14:34Z

Looks like a missing eslint configuration in the tilegarden source directory?

Whoops, had it locally but had neglected to add it. Since I was rebasing that in anyway, I went ahead and resolved the conflicts to make it look like the inspect port was never changed...

KlaasH assigned CloudNiner Nov 30, 2018

KlaasH requested a review from CloudNiner November 30, 2018 22:03

KlaasH added the in review label Nov 30, 2018

CloudNiner reviewed Dec 3, 2018

View reviewed changes

CloudNiner approved these changes Dec 3, 2018

View reviewed changes

KlaasH added 14 commits December 3, 2018 14:48

Copy Tilegarden in, pruning but not modifying

cc93aa1

Copies the parts of Tilegarden that will be needed here into this project, pruning a bit (but not extensively--there may still be some cruft) and changing identifiers in package.json but not making any substantive changes.

Replace hand-promisified 'fs' with util.promisify-ed one

19e94ed

Especially since we're only actually using one of the functions in one place, just using `promisify` in tiler.js is simple and works fine.

Switch claudia-local-api dependency to Azavea fork

58edb23

Matt DelSordo's fork has some fixes that are needed for Tilegarden to work properly. This switches to a fork of that that we can continue developing on.

Add yarn.lock

731e1b0

Get ready for gigantic diffs for tiny changes!

Adjust map-config MML for PFB database

f3e35de

Add tilegarden container to scripts/console

6918cdd

When running as a development server, it can be useful to get a shell.

Return a special error message when there's no default config

b071e83

An unqualified tile URL will produce a somewhat unhelpful message if there's no default config file. This catches errors from opening the config file and sets a more helpful message in that situation.

Remove vector and grid tile code

6cfba32

We're not going to be using utfGrids or vector tiles here, so there's no reason to keep the code around in this repo (we could always copy it back in if we change our mind).

Add tilegarden to 'update' and 'server' scripts

2a38e77

Doesn't remove the old 'tilemaker' yet, since it hasn't been replaced yet elsewhere in the code.

Prune, fix, and run Tilegarden tests

2e7d850

Adds the Tilegarden test command to `scripts/test` and gets the tests running by removing a few tests of removed code and fixing up the imports for the shift to non-transpiled code.

KlaasH force-pushed the feature/kjh/add-tilegarden branch from 3634c1e to 2e7d850 Compare December 3, 2018 19:50

KlaasH merged commit 7f4e2b7 into develop Dec 3, 2018

hectcastro removed the in review label Dec 3, 2018

KlaasH deleted the feature/kjh/add-tilegarden branch December 3, 2018 20:15

KlaasH mentioned this pull request Dec 4, 2018

Enable Tilegarden tiler and add job_id filtering #613

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tilegarden #610

Add tilegarden #610

KlaasH commented Nov 30, 2018

CloudNiner left a comment

CloudNiner Dec 3, 2018

CloudNiner Dec 3, 2018

KlaasH Dec 3, 2018

CloudNiner Dec 3, 2018

KlaasH commented Dec 3, 2018

CloudNiner left a comment

KlaasH commented Dec 3, 2018

Add tilegarden #610

Add tilegarden #610

Conversation

KlaasH commented Nov 30, 2018

Overview

Demo

Notes

Testing Instructions

CloudNiner left a comment

Choose a reason for hiding this comment

CloudNiner Dec 3, 2018

Choose a reason for hiding this comment

CloudNiner Dec 3, 2018

Choose a reason for hiding this comment

KlaasH Dec 3, 2018

Choose a reason for hiding this comment

CloudNiner Dec 3, 2018

Choose a reason for hiding this comment

KlaasH commented Dec 3, 2018

CloudNiner left a comment

Choose a reason for hiding this comment

KlaasH commented Dec 3, 2018