feat(airflow): OWID, FOPH and Colombia dag. #133

luabida · 2022-09-01T13:00:49Z

Start of Airflow implementation. It still needs improvements on the documentation and some bug fixes.

How to test the changes:

make docker-build ENV=dev
make docker-start ENV=dev
access http://127.0.0.1:8099/
login with admin, password admin
trigger manually the DAGs, you can find the workflow in Graph page.

NOTES:

Sometimes the Scheduler won't boot with the Webserver, I'm doing it manually inside Airflow's container with the command airflow scheduler (ping @xmnlab)
In FOPH Dag have some tasks that won't match the tables size, it needs to be fixed.
foph_casesvaccpersons_d table in FOPH Dag fails.
Tests need to be implemented.
In the future, all modules will import directly from epigraphhub_py.

fix #36
fix #120
fix #137

xmnlab

thanks for working on that @luabida .

I think we should move data_collection to epigraphhub_py earlier than later, so I vote to move that before we merge this PR.

the other comments is most about folders and environment variables

data_collection/config.py

data_collection/foph/download_foph_data.py

data_collection/owid/compare_data.py

docker/airflow/Dockerfile

docker/airflow/dags/colombia_dag.py

docker/airflow/dags/foph_dag.py

docker/airflow/dags/owid_dag.py

docker/superset/Dockerfile

xmnlab · 2022-09-01T15:12:30Z

@luabida some extra comments about the environment folders:

inside a library that is not good to have a folder path defined hard coded, for example, if the user want to use the library in a different operation system or with restricted access to the system's folder. so it is very important to have a way to parametrize that.

.env files are very nice for system or platforms .. but not good for libraries ... so for libraries we maybe would like to have something like a config file .. maybe something inside ${HOME}/.config/epigraphhub.cnf

let's discuss that today or tomorrow, so we can define that as soon as possible

eduardocorrearaujo · 2022-09-01T17:11:20Z

thanks for working on that @luabida .

I think we should move data_collection to epigraphhub_py earlier than later, so I vote to move that before we merge this PR.

the other comments is most about folders and environment variables

I agree

data_collection/foph/foph_fetch.py

eduardocorrearaujo · 2022-09-01T18:20:42Z

docker/airflow/dags/colombia_dag.py

+        task_id="start",
+    )
+
+    @task(task_id="load_into_db", retries=2)


In this link: https://dev.socrata.com/foundry/www.datos.gov.co/gt2j-8ykr/ , it's available the last updated date of this dataset. Unfortunately, I don't find a way to get it directly from the API. But, maybe we can create a web scraping to get it and use it as a way to decide if the code to update data should run or not. I can do it.

@luabida, a final comment: Following issue #129, you could create a task that, after updating the table, will add (or update) the date of update of this table in the meta table.

Thank you so much for this first review, I'll patching the changes in this PR so we can track the TODOs for the Airflow.

fccoelho · 2022-09-02T13:11:25Z

When relocating the code to the epigraphhub_py library, please open a PR there so that we can check that we have reasonable test coverage. This code is critical and must be carefully tested.

luabida · 2022-09-02T20:39:39Z

ATTENTION! The last commit possibly BREAKS dependencies, it was the only way I could import epigraphhub_py library, which has a lot of dependency problems with EpiGraphHub.

conda/pip.txt

docker/airflow/dags/owid_dag.py

luabida · 2022-09-06T01:14:36Z

Currently using this branch to install the eph lib: https://github.com/luabida/epigraphhub_py/tree/data-collection
I'll start the documentation in next commits.
Some foph csv still need fix in the compare data step.

luabida · 2022-09-08T21:55:33Z

#ec337b5 commit is the beginning of airflow's documentations, more information will be included in next commits

fccoelho · 2022-09-19T13:44:39Z

Documentation/airflow.md

@@ -0,0 +1,103 @@
+# Airflow Overview


You need to create an entry for this document on index.rst Perhaps named: "Creating Data Collection DAGS". Maybe it can replace this page, but first take a look at it and move any still relevant information to this page.

luabida · 2022-09-23T15:18:47Z

@xmnlab about #08e8f98: I had to configure the .yaml file by hand changing the host variable to the postgres container's IP address. Could it be a Docker network error?

luabida · 2022-10-11T18:17:39Z

about #4f4ee21:
I'm not quite sure how the DAGs that updates the tables should be tested nor if unity tests for tables existence should be added along with the tables Airflow is meant to update.

Only the dag that fix #137 remained.

xmnlab · 2022-10-11T20:15:40Z

about #4f4ee21: I'm not quite sure how the DAGs that updates the tables should be tested nor if unity tests for tables existence should be added along with the tables Airflow is meant to update.

Only the dag that fix #137 remained.

according to this: https://stackoverflow.com/a/59316781/4995866

I think that you would have two options:

CLI: Run airflow trigger_dag <dag_id>, see docs in https://airflow.apache.org/docs/stable/cli.html#trigger_dag. Note that later versions of airflow use the syntaxairflow dags trigger <dag_id>
API: Call POST /api/experimental/dags/<dag_id>/dag_runs, see docs in https://airflow.apache.org/docs/stable/api.html#post--api-experimental-dags--DAG_ID--dag_runs.

with api call post, maybe you could run that outside of the airflow container, although, not sure about timeout or the response.

if you decide to use CLI, you can do something like:

add to Makefile:

CMD:=

.PHONY:docker-exec-command
docker-exec-command:
	$(DOCKER) exec ${SERVICE} ${CMD}

and call it with: make docker-exec-command CMD="airflow trigger_dag <dag_id>"

does it make sense?

luabida · 2022-10-11T20:20:33Z

It does make sense, thank you for helping on this subject. Another issue is the credentials on CI for the legacy Cron tests. Do you know why it receives permissions error?
https://github.com/thegraphnetwork/EpiGraphHub/actions/runs/3229208852/jobs/5286235535#step:9:114

xmnlab · 2022-10-11T20:25:12Z

not sure .. maybe you need to review the sql files to ensure that the user used to create the table has the permissions.
but probably it is safe to remove the cron tests step because it will be replaced by your DAGS

fccoelho · 2022-10-11T20:32:47Z

about #4f4ee21: I'm not quite sure how the DAGs that updates the tables should be tested nor if unity tests for tables existence should be added along with the tables Airflow is meant to update.

Only the dag that fix #137 remained.

I think we need to be careful to avoid redundant testing: If a DAG uses code that is implemented in the epigraphhub library, that code should not be tested (or re-tested) from inside the DAG, but rather directly in the library where it is much easier. The only testing required from airflow are that it starts and finishes the DAGs correctly.

fccoelho · 2022-10-11T20:43:26Z

It does make sense, thank you for helping on this subject. Another issue is the credentials on CI for the legacy Cron tests. Do you know why it receives permissions error? https://github.com/thegraphnetwork/EpiGraphHub/actions/runs/3229208852/jobs/5286235535#step:9:114

When I fixed the permissions for the epigraph user that does the insertions on the cron script, I remember that I had to grant permissions also at the schema level, not just for the table...

fccoelho · 2022-10-11T20:49:09Z

note that the error:

sqlalchemy.exc.ProgrammingError: (psycopg2.errors.InsufficientPrivilege) permission denied for schema switzerland
LINE 2: CREATE TABLE switzerland.foph_cases_d

is for schema switzerland, because you are trying to create a table, so you probably need something like this:

GRANT USAGE ON  SCHEMA <name_of the_schema> TO <user> ;
GRANT ALL ON ALL TABLES IN SCHEMA <name_of the_schema> TO <user> ;
GRANT ALL ON ALL FUNCTIONS IN SCHEMA <name_of the_schema> TO <user> ;
GRANT ALL ON ALL SEQUENCES IN SCHEMA <name_of the_schema> TO <user> ;

maybe something more...

esloch · 2022-10-12T11:57:48Z

Everyone, in the makefile we have a target make docker-dev-prepare-db. I didn't find it in any step of the CI.
How are sql scripts imported into docker-entrypoint-initdb.d in postgres container?

xmnlab · 2022-10-12T13:12:36Z

It should be done automatically by dockerfile/docker-compose. Is it not working?

esloch · 2022-10-12T15:15:52Z

It should be done automatically by dockerfile/docker-compose. Is it not working?

Thanks Ivan, initdb is up to date with a volume in the compose file:

      volumes:
        - ./postgresql/sql/dev/:/docker-entrypoint-initdb.d/

I will continue to debug on my localhost.

…lure to True on DAGs

xmnlab

Thanks @luabida , I will take another later today

luabida · 2022-10-13T17:53:12Z

fix #36
fix #120
fix #137

xmnlab · 2022-10-14T14:57:15Z

fix #36 fix #120 fix #137

@luabida can you add this information directly to the first comment of this PR? otherwise github will not close these issues automatically

luabida · 2022-10-14T14:58:38Z

Done. Is it done to merge? From my side there is the other PR that needs rebasing from main after this merge

xmnlab

just added a few comments, other than that LGTM.
thanks @luabida !

xmnlab · 2022-10-14T14:58:40Z

Makefile

@@ -86,8 +86,8 @@ docker-dev-prepare-db:

 .PHONY:docker-run-cron


can you remove the docker-run-cron target pls? it will not be used anymore

xmnlab · 2022-10-14T14:59:44Z

Makefile


 .PHONY:docker-console
 docker-console:
-	$(DOCKER) exec ${SERVICE} ${CONSOLE}
+	$(DOCKER) exec -it ====${SERVICE} ${CONSOLE}


do we need this bunch of equal sign here?

xmnlab · 2022-10-14T15:01:16Z

docker/superset/Dockerfile

@@ -78,7 +78,8 @@ COPY --chown=epigraphhub:epigraphhub docker/superset/entrypoint.sh /opt/entrypoi
 RUN chmod +x /opt/entrypoint.sh \
  && echo "source /opt/entrypoint.sh" > ~/.bashrc \
  && sudo mkdir -p /opt/data/superset/ \
-  && sudo chown -R epigraphhub:epigraphhub /opt/data


any reason for removing this one?

luabida · 2022-10-14T15:02:30Z

Another question is the user creation for Airflow, maybe we will need a bit of caution on how is it rn

xmnlab

LGTM! thanks @luabida !

xmnlab · 2022-10-14T15:39:36Z

Another question is the user creation for Airflow, maybe we will need a bit of caution on how is it rn

could explain a bit more about that pls?

xmnlab · 2022-10-14T15:40:33Z

btw, the database issue we are planning to fix with #142

luabida · 2022-10-14T15:47:35Z

could explain a bit more about that pls?

I wasn't sure how would be done the Users on Airflow webserver. The data collection DAGs are set with the owner: epigraphhub. It doesn't stop the Admin to run the DAGs, but if some dev get access from a user that is not Admin, it has to be 'epigraphhub', otherwise it won't show any DAG

luabida and others added 7 commits August 24, 2022 19:19

Begin of owid workflow

5eefb19

Add logger and clean data files do owid dag

3d25ae1

Colombia data DAG

7eb5266

Add foph DAG

66d214e

Run black

27a338f

DEBUG log, some table sizes won't ever match

c757d4f

Remove duplicated file

eee97b6

xmnlab reviewed Sep 1, 2022

View reviewed changes

eduardocorrearaujo reviewed Sep 1, 2022

View reviewed changes

DRAFT test epigraphhub_py import

7514ec8

xmnlab reviewed Sep 2, 2022

View reviewed changes

conda/pip.txt Outdated Show resolved Hide resolved

docker/airflow/dags/owid_dag.py Outdated Show resolved Hide resolved

luabida added 3 commits September 2, 2022 19:20

foph dag

0a7be2d

Remove old data

2aaa60d

Finish colombia data

dcf79fe

luabida added 3 commits September 6, 2022 15:12

Reduce comparation bugs

e502605

Minor fixes

22dfc71

Add stdout logging to DAGs

bb45e1f

luabida force-pushed the owid-dag branch from c34e33d to bb45e1f Compare September 8, 2022 11:52

Start of Airflow Documentation

ec337b5

fccoelho reviewed Sep 19, 2022

View reviewed changes

luabida added 2 commits September 23, 2022 12:16

Configuring the postgres connection with .config/epigraphhub.yaml file

08e8f98

Add documentation for owid dag

caa2e13

FOPH docstrings plus minor adjustments

0a207f9

Remove database tests

4f4ee21

luabida marked this pull request as ready for review October 11, 2022 19:14

Fix permissions for legacy Cron scripts

ae3e2f8

luabida force-pushed the owid-dag branch from c56e72a to ae3e2f8 Compare October 11, 2022 21:26

Removing legacy Data Collection Cron scripts and setting Email on Fai…

e595420

…lure to True on DAGs

fccoelho approved these changes Oct 13, 2022

View reviewed changes

Removing forecast code. Deprecated.

589dcb4

xmnlab reviewed Oct 13, 2022

View reviewed changes

luabida mentioned this pull request Oct 13, 2022

docs(airflow): Airflow DAG request issue template #145

Merged

xmnlab requested changes Oct 14, 2022

View reviewed changes

Fix typos

806b381

xmnlab approved these changes Oct 14, 2022

View reviewed changes

xmnlab merged commit e854993 into thegraphnetwork:main Oct 14, 2022

		@@ -86,8 +86,8 @@ docker-dev-prepare-db:

		.PHONY:docker-run-cron

feat(airflow): OWID, FOPH and Colombia dag. #133

feat(airflow): OWID, FOPH and Colombia dag. #133

Conversation

luabida commented Sep 1, 2022 • edited Loading

xmnlab left a comment

Choose a reason for hiding this comment

xmnlab commented Sep 1, 2022

eduardocorrearaujo commented Sep 1, 2022

eduardocorrearaujo Sep 1, 2022

Choose a reason for hiding this comment

eduardocorrearaujo Sep 1, 2022

Choose a reason for hiding this comment

luabida Sep 1, 2022

Choose a reason for hiding this comment

fccoelho commented Sep 2, 2022

luabida commented Sep 2, 2022

luabida commented Sep 6, 2022

luabida commented Sep 8, 2022

fccoelho Sep 19, 2022

Choose a reason for hiding this comment

luabida commented Sep 23, 2022

luabida commented Oct 11, 2022

xmnlab commented Oct 11, 2022

luabida commented Oct 11, 2022

xmnlab commented Oct 11, 2022

fccoelho commented Oct 11, 2022

fccoelho commented Oct 11, 2022

fccoelho commented Oct 11, 2022 • edited Loading

esloch commented Oct 12, 2022

xmnlab commented Oct 12, 2022

esloch commented Oct 12, 2022

xmnlab left a comment • edited Loading

Choose a reason for hiding this comment

luabida commented Oct 13, 2022

xmnlab commented Oct 14, 2022

luabida commented Oct 14, 2022 • edited Loading

xmnlab left a comment

Choose a reason for hiding this comment

xmnlab Oct 14, 2022

Choose a reason for hiding this comment

xmnlab Oct 14, 2022

Choose a reason for hiding this comment

xmnlab Oct 14, 2022

Choose a reason for hiding this comment

luabida commented Oct 14, 2022

xmnlab left a comment

Choose a reason for hiding this comment

xmnlab commented Oct 14, 2022

xmnlab commented Oct 14, 2022

luabida commented Oct 14, 2022 • edited Loading

luabida commented Sep 1, 2022 •

edited

Loading

fccoelho commented Oct 11, 2022 •

edited

Loading

xmnlab left a comment •

edited

Loading

luabida commented Oct 14, 2022 •

edited

Loading

luabida commented Oct 14, 2022 •

edited

Loading