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

Allow country to be set using quickstart #5797

Merged
merged 5 commits into from
Feb 20, 2023
Merged

Conversation

greenape
Copy link
Member

@greenape greenape commented Jan 23, 2023

Closes #5796

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

Allows setting the country used for synthetic data with quickstart script.


@greenape greenape added enhancement New feature or request FlowDB Issues related to FlowDB quick-start-script labels Jan 23, 2023
@greenape greenape requested a review from jc-harrison January 23, 2023 16:34
@cypress
Copy link

cypress bot commented Jan 23, 2023

Passing run #18421 ↗︎

0 4 0 0 Flakiness 0

Details:

Fix env var name
Project: FlowAuth Commit: d305394457
Status: Passed Duration: 00:57 💡
Started: Feb 20, 2023 11:34 AM Ended: Feb 20, 2023 11:35 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Merging #5797 (d305394) into master (d8777b9) will decrease coverage by 14.85%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #5797       +/-   ##
===========================================
- Coverage   93.23%   78.39%   -14.85%     
===========================================
  Files         277      277               
  Lines       10828    10828               
  Branches      895      895               
===========================================
- Hits        10096     8489     -1607     
- Misses        602     2095     +1493     
- Partials      130      244      +114     
Impacted Files Coverage Δ
...e/flowmachine/core/sqlalchemy_table_definitions.py 0.00% <0.00%> (-100.00%) ⬇️
...wmachine/features/subscriber/active_subscribers.py 0.00% <0.00%> (-100.00%) ⬇️
...ne/features/subscriber/per_subscriber_aggregate.py 0.00% <0.00%> (-100.00%) ⬇️
...e/features/utilities/unique_values_from_queries.py 0.00% <0.00%> (-100.00%) ⬇️
...owmachine/features/utilities/feature_collection.py 23.80% <0.00%> (-76.20%) ⬇️
flowmachine/flowmachine/core/geotable.py 28.57% <0.00%> (-71.43%) ⬇️
...flowmachine/features/subscriber/distance_series.py 30.00% <0.00%> (-70.00%) ⬇️
...e/flowmachine/features/subscriber/handset_stats.py 21.95% <0.00%> (-68.30%) ⬇️
...es/subscriber/contact_reference_locations_stats.py 33.33% <0.00%> (-66.67%) ⬇️
flowmachine/flowmachine/core/mixins/graph_mixin.py 25.00% <0.00%> (-65.00%) ⬇️
... and 102 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jc-harrison
Copy link
Member

For some reason when I tried this, it didn't start flowmachine. Probably a weird glitch on my end - I'll try again.

@flowstef
Copy link
Contributor

Tried this but it still doesn't work for me. I can run the script but once it's up I still get NPL admin areas in the example notebooks.

@jc-harrison
Copy link
Member

Tried this but it still doesn't work for me. I can run the script but once it's up I still get NPL admin areas in the example notebooks.

@flowstef Note that to test this you'll need to export GIT_REVISION as the commit hash of the commit you want to test, otherwise it will use the compose files from master. Just pointing this out, because I forgot this step...

@flowstef
Copy link
Contributor

I tried both with the GIT_REVISION var and running it just by checking out the correct branch and using the local quick_start.sh as opposed to the one from github.

@greenape
Copy link
Member Author

Ah, my bad - the environment variables provided are being overridden by the downloaded development_environment ones.

@greenape greenape force-pushed the add-country-to-quickstart branch from ae3b38a to 70007c5 Compare January 24, 2023 11:07
Copy link
Member

@jc-harrison jc-harrison left a comment

Choose a reason for hiding this comment

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

Still failing to start flowmachine, but I can't see how that could be related to these changes. I'm getting errors when I run the script:

Starting containers (this may take a few minutes)
Pulling worked_examples          ... done
Pulling flowauth                 ... done
Pulling flowapi                  ... done
Pulling flowdb                   ... done
Pulling flowmachine_query_locker ... done
Pulling flowmachine              ... done
Creating network "flowkit_qs_default" with the default driver
Creating network "flowkit_qs_flowetl" with the default driver
Creating network "flowkit_qs_zero" with the default driver
Creating network "flowkit_qs_api" with the default driver
Creating network "flowkit_qs_db" with the default driver
Creating network "flowkit_qs_redis" with the default driver
Creating worked_examples ...
Creating flowdb          ...
Creating flowapi         ...
Creating flowauth        ...
Creating flowmachine_query_locker ...

ERROR: for flowmachine_query_locker  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=60)

ERROR: for flowdb  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=60)

ERROR: for flowauth  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=60)

ERROR: for flowapi  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=60)

ERROR: for worked_examples  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=60)

ERROR: for flowmachine_query_locker  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=60)

ERROR: for flowdb  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=60)

ERROR: for flowauth  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=60)

ERROR: for flowapi  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=60)

ERROR: for worked_examples  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=60)
ERROR: An HTTP request took too long to complete. Retry with --verbose to obtain debug information.
If you encounter this issue regularly because of slow network conditions, consider setting COMPOSE_HTTP_TIMEOUT to a higher value (current value: 60).

After waiting a while everything starts up OK despite the error messages, except that there's no sign of flowmachine.

But flowdb does have data for the specified country. I don't think the disaster region pcod is working, though - with COUNTRY=GBR and DISASTER_REGION_PCOD=GBR.1.36_1, looking at modal locations in the 2 weeks starting 2016-02-11 (the default disaster start date), there are still a normal number of residents in Greater London.
Screenshot 2023-01-26 at 12 00 31

@jc-harrison
Copy link
Member

Ah, I just re-read the docs change and realised I should be using EXAMPLE_COUNTRY and EXAMPLE_DISASTER_REGION_PCOD. COUNTRY appeared to work as expected though, although DISASTER_REGION_PCOD didn't.

quick_start.sh Outdated Show resolved Hide resolved
@greenape greenape force-pushed the add-country-to-quickstart branch from c488dbb to b7247d0 Compare January 30, 2023 11:51
@greenape greenape added the ready-to-merge Label indicating a PR is OK to automerge label Jan 30, 2023
@greenape greenape force-pushed the add-country-to-quickstart branch from b7247d0 to 2ad3306 Compare February 10, 2023 10:39
@greenape greenape requested a review from jc-harrison February 10, 2023 10:55
@greenape greenape force-pushed the add-country-to-quickstart branch from 2ad3306 to a7b34e5 Compare February 10, 2023 15:03
@greenape greenape force-pushed the add-country-to-quickstart branch from a7b34e5 to 8b8267e Compare February 20, 2023 09:40
@greenape greenape force-pushed the add-country-to-quickstart branch from 8b8267e to d305394 Compare February 20, 2023 11:25
@greenape greenape merged commit c1b5915 into master Feb 20, 2023
@greenape greenape deleted the add-country-to-quickstart branch February 20, 2023 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request FlowDB Issues related to FlowDB quick-start-script ready-to-merge Label indicating a PR is OK to automerge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

COUNTRY not available via quick start
3 participants