Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dockerized worked examples #694

Merged
merged 31 commits into from
May 8, 2019
Merged

Dockerized worked examples #694

merged 31 commits into from
May 8, 2019

Conversation

greenape
Copy link
Member

@greenape greenape commented May 2, 2019

Closes #614 closes #688

I have:

  • Formatted any Python files with black
  • Brought the branch up to date with master
  • Added any relevant Github labels
  • Added tests for any new additions
  • Added or updated any relevant documentation
  • Added an Architectural Decision Record (ADR), if appropriate
  • Added an MPLv2 License Header if appropriate
  • Updated the Changelog

Description

Overall intent: make the worked examples super easy to run.

  • Adds a new top level Dockerfile based on the official jupyter images, with the worked examples from the docs built in, and flowmachine and FlowClient installed.
  • Adds worked examples to the quick start bash script
  • Squashes the CI build process down a bit so that all the docker images can be built earlier because there's no particular reason to wait until the corresponding tests pass to try and build them

@greenape greenape added enhancement New feature or request docs Documentation issues labels May 2, 2019
@greenape
Copy link
Member Author

greenape commented May 2, 2019

To try this out prior to merging to master, you can run:

CI=true CIRCLE_SHA1=21fc0dbb7f3d79391bf736e30e38ea5c80710a25 bash <(curl -s https://raw.githubusercontent.com/Flowminder/FlowKit/21fc0dbb7f3d79391bf736e30e38ea5c80710a25/quick_start.sh) examples

(Once the currently running CI process finished)

@codecov
Copy link

codecov bot commented May 2, 2019

Codecov Report

Merging #694 into master will decrease coverage by 5.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #694      +/-   ##
==========================================
- Coverage   93.65%   88.55%   -5.11%     
==========================================
  Files         127       13     -114     
  Lines        6384     1083    -5301     
  Branches      677        0     -677     
==========================================
- Hits         5979      959    -5020     
+ Misses        283      124     -159     
+ Partials      122        0     -122
Impacted Files Coverage Δ
flowapi/flowapi/api_spec.py 25.53% <0%> (-68.09%) ⬇️
flowclient/flowclient/client.py 83.57% <0%> (-10.63%) ⬇️
flowapi/flowapi/query_endpoints.py 84.37% <0%> (-6.25%) ⬇️
flowapi/flowapi/user_model.py 93.58% <0%> (-3.85%) ⬇️
...e/server/query_schemas/joined_spatial_aggregate.py
...achine/core/server/query_schemas/daily_location.py
...e/flowmachine/features/raster/raster_statistics.py
flowmachine/flowmachine/core/custom_query.py
flowmachine/flowmachine/core/server/server.py
...hine/features/location/unique_subscriber_counts.py
... and 108 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 f92262e...f979adc. Read the comment docs.

@codecov
Copy link

codecov bot commented May 2, 2019

Codecov Report

Merging #694 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #694   +/-   ##
=======================================
  Coverage   93.68%   93.68%           
=======================================
  Files         129      129           
  Lines        6412     6412           
  Branches      678      678           
=======================================
  Hits         6007     6007           
  Misses        283      283           
  Partials      122      122
Impacted Files Coverage Δ
flowauth/backend/flowauth/models.py 93.29% <ø> (ø) ⬆️

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 87cc59a...7033cf4. Read the comment docs.

@jc-harrison
Copy link
Member

A few issues I've come across so far:

  • The FlowAPI URL set in flowclient.connect() in the example notebooks is "http://localhost:9090", which is correct for the docs build, but inside the container this needs to be "http://flowapi:9090".
  • The API token available to TEST_USER in the demo FlowAuth setup only allows access to admin0 and admin1 aggregations, but the queries in the worked examples aggregate to admin3 level, so a user would have to log into FlowAuth as TEST_ADMIN and modify TEST_USER's permissions before being able to create a token which allows them to run the examples.
  • By default, bash <(curl ...) examples starts the system with flowdb_testdata, but the worked examples use date ranges not included in the test data so attempting to run them will give "No data for date" errors.

Makefile Outdated
@@ -108,6 +108,16 @@ flowauth-build:
docker-compose -f $(DOCKER_COMPOSE_FILE) -f $(DOCKER_COMPOSE_FILE_BUILD) build flowauth


worked-examples-up: worked-examples-build
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worked_examples-up instead of worked-examples-up would be more consistent with the names of the other targets.

for the examples with a small dataset, or

```bash
bash <(curl -s https://raw.githubusercontent.com/Flowminder/FlowKit/master/quick_start.sh) examples larger_data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be useful to also give instructions here for stopping the system cleanly (presumably bash ... stop is sufficient).

@greenape
Copy link
Member Author

greenape commented May 3, 2019

Ok, let’s -

  • make the url an env var
  • make the default token more permissive
  • switch the default flowdb to synth for the examples and allow making it smaller

I had it as synth originally, but it takes soooo long to spin up on ci that I swapped it to the testdata.

@greenape greenape force-pushed the dockerized-worked-examples branch from d97b49f to 3418e72 Compare May 7, 2019 08:39
@greenape greenape force-pushed the dockerized-worked-examples branch from e3fa017 to 21fc0db Compare May 7, 2019 10:14
quick_start.sh Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@maxalbert maxalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant, very nice work! 👏

Great to have the worked examples readily available. 🎉

When I tried to execute them inside the docker containers there were few errors, but they have nothing to do with this PR (basically we haven't upgraded them to more recent versions of flowclient, and a couple of things could be done more conveniently). So let's address this separately (and it may already be part of what @jch1g10 is working on).

Co-Authored-By: greenape <[email protected]>
@greenape greenape added the ready-to-merge Label indicating a PR is OK to automerge label May 8, 2019
@mergify mergify bot merged commit beafbbb into master May 8, 2019
@mergify mergify bot deleted the dockerized-worked-examples branch May 8, 2019 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation issues enhancement New feature or request ready-to-merge Label indicating a PR is OK to automerge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quick install instructions are insufficient Demo mode friendly docker-compose file
3 participants