feat(dag): add data transfer task group for release process #528

DSuveges · 2024-03-07T23:51:14Z

✨ Context

Some updates were made for the next data release + minor improvements in the Airflow layer. Scope defined under #3238.

🛠 What does this PR implement

Updated configurations for release 24.03
Targest and interactions are considered as static assests (the airflow service account has no access to platform buckets.)
The ETL DAG now transfers required datasets from the datasource specific buckets.
FINNGEN susie ingestion outputs to the canonical location.

🚦 Before submitting

Do these changes cover one single feature (one change at a time)?
Did you read the contributor guideline?
Did you make sure to update the documentation with your changes?
Did you make sure there is no commented out code in this PR?
Did you follow conventional commits standards in PR title and commit messages?
Did you make sure the branch is up-to-date with the dev branch?
Did you write any new necessary tests?
Did you make sure the changes pass local tests (make test)?
Did you make sure the changes pass pre-commit rules (e.g poetry run pre-commit run --all-files)?

chore(l2g): update inclusion features list with newer QTLs

ireneisdoomed

Thank you!
Changes here mostly concern adding a group of tasks that place all necessary files in the release bucket prior to the ETL run. Therefore, I'd suggest changing the PR title to sth like feat(dag): add data transfer task group for release process
This is the ETL DAG now:

I made some comments, let me know your thoughts. Overall, I like the implementation and the idea of introducing the ShortCiruitOperator to safeguard ourselves from rerunning unnecessary processes is very nice.

ireneisdoomed · 2024-03-12T10:44:47Z

config/datasets/ot_gcp.yaml

@@ -36,9 +36,9 @@ anderson: ${datasets.static_assets}/andersson2014/enhancer_tss_associations.bed
 javierre: ${datasets.static_assets}/javierre_2016_preprocessed
 jung: ${datasets.static_assets}/jung2019_pchic_tableS3.csv
 thurman: ${datasets.static_assets}/thurman2012/genomewideCorrs_above0.7_promoterPlusMinus500kb_withGeneNames_32celltypeCategories.bed8.gz
-target_index: ${datasets.release_folder}/targets # OTP 23.12 data
+target_index: ${datasets.static_assets}/targets # OTP 23.12 data


Would it make sense to just point to our OTP release bucket?

ireneisdoomed · 2024-03-12T10:44:56Z

config/datasets/ot_gcp.yaml

@@ -36,9 +36,9 @@ anderson: ${datasets.static_assets}/andersson2014/enhancer_tss_associations.bed
 javierre: ${datasets.static_assets}/javierre_2016_preprocessed
 jung: ${datasets.static_assets}/jung2019_pchic_tableS3.csv
 thurman: ${datasets.static_assets}/thurman2012/genomewideCorrs_above0.7_promoterPlusMinus500kb_withGeneNames_32celltypeCategories.bed8.gz
-target_index: ${datasets.release_folder}/targets # OTP 23.12 data
+target_index: ${datasets.static_assets}/targets # OTP 23.12 data
+gene_interactions: ${datasets.static_assets}/interaction # OTP 23.12 data


Would it make sense to just point to our OTP release bucket?

It might makes sense, however the service account can't read that bucket. And I don't see a reason to extend its permission to other buckets. In a way, while the platform and the genetics etls are separated, this solution is fine.

src/airflow/dags/test_DAG.py

ireneisdoomed · 2024-03-12T10:57:16Z

src/airflow/dags/test_DAG.py

+
+# Datasource paths:
+GWAS_CATALOG_BUCKET_NAME = "gwas_catalog_data"
+EQTL_BUCKET_NAME = "eqtl_catalog_data"


It doesn't concern this PR and it's incredibly minor, but it annoys me a bit that the source name is incorrect. Do you think we should rename it to eqtl_catalogue_data?

Yes, totally makes sense. It must be that the US form was derived from the GWAS Catalog.

because of funding as far as I understand

ireneisdoomed · 2024-03-12T11:24:49Z

src/airflow/dags/test_DAG.py

+
+
+# Test if release folder exists:
+def test_release_folder_exists() -> bool:


This util might be useful as a sensor in other instances.
You could parametrise it and move it to common_airflow.py

def test_release_folder_exists(bucket: str, path: str) -> bool: """This function tests if the release folder exists. Args: bucket (str): Name of the bucket in GCS path (str): Object name in GCS to check Returns: bool: False if the folder exists, True otherwise. """ hook = GCSHook(gcp_conn_id="google_cloud_default") return not hook.exists(bucket, path)

Minor, but the GCSHook connector is interesting

Agree, it makes sense to generalise.

src/airflow/dags/test_DAG.py

Co-authored-by: Irene López <45119610+ireneisdoomed@users.noreply.github.com>

ireneisdoomed

Thank you for addressing the comments!

ireneisdoomed · 2024-03-13T12:04:26Z

src/airflow/dags/common_airflow.py

@@ -59,6 +60,24 @@
 }


+# Test if release folder exists:


Suggested change

# Test if release folder exists:

Unfortunately we are still not there yet. (that's why I haven't ask for a re-reivew) For some reason the the gcshook.exists() stopped working. Now I'm trying to find out what is the problem. Keep you posted.

I see, thanks for testing it.

…ts/gentropy into ds_3238_update_ETL_DAG

DSuveges · 2024-03-14T10:32:02Z

src/airflow/dags/genetics_etl.py

+    python_callable=lambda bucket, path: not common.check_gcp_folder_exists(
+        bucket, path
+    ),


It might be controversial, but I'm up for defending this decision: I don't like to write separate functions for testing the presence and for testing the absence of something. I like the idea to test something positive, because that's eaiser to understand eg. the tested folder is there -> returns True.

However it implies that if we need to test the absence, we have to flip the boolean value. We can either flip the return value or make the function flexible to decide if we want the presence or the absence. I don't like the latter solution. This means if I'm using an airflow operator on a python callable, I need to use lambda. It might not be the nicest, but I think this is the right choice here.

Not controversial to me. Functions are meant to be reused. Thanks!

ireneisdoomed · 2024-03-14T11:23:50Z

Thank you for the changes! Was the hook failing or it simply stopped working?

DSuveges · 2024-03-14T12:10:24Z

Was the hook failing or it simply stopped working?

Apparently the GCSHook.exists() cannot be used to test if a folder exists or not, as there are no such concepts in google cloud explained by this SO post. Instead I need to list the content of a bucket and evaluate if there are content in the release folder. Not as nice, but kind of make sense.

DSuveges and others added 7 commits March 7, 2024 23:36

chore: resolve merge commit

c2e3e91

chore: bumping for 24.03 release

9b64cf8

fix: reverting some changes

9ee712a

chore: add tuqtl features to L2Gfeaturematrix schema

6e62c4b

chore: add sqtl, tuqtl, pqtl features to l2g inclusion list

fda650b

Merge pull request #529 from opentargets/il-l2g-config-changes

4dcf542

chore(l2g): update inclusion features list with newer QTLs

Merge branch 'dev' into ds_3238_update_ETL_DAG

f12d4ea

DSuveges marked this pull request as ready for review March 11, 2024 20:24

DSuveges requested a review from ireneisdoomed March 12, 2024 10:01

ireneisdoomed reviewed Mar 12, 2024

View reviewed changes

DSuveges and others added 2 commits March 12, 2024 17:05

Merge branch 'dev' into ds_3238_update_ETL_DAG

088895a

chore: apply suggestions from code review

db73c23

Co-authored-by: Irene López <45119610+ireneisdoomed@users.noreply.github.com>

DSuveges changed the title ~~chore: update for 24.03 data release~~ eat(dag): add data transfer task group for release process Mar 13, 2024

DSuveges changed the title ~~eat(dag): add data transfer task group for release process~~ feat(dag): add data transfer task group for release process Mar 13, 2024

refactor: tidying up the ETL DAG

6fa81a6

ireneisdoomed approved these changes Mar 13, 2024

View reviewed changes

Merge branch 'dev' into ds_3238_update_ETL_DAG

cd3324a

github-actions bot added size-M Step Feature airflow labels Mar 13, 2024

DSuveges added 2 commits March 14, 2024 10:23

fix(dag): sorting out the checks for the existence of GCP folders

cae2e40

Merge branch 'ds_3238_update_ETL_DAG' of https://github.com/opentarge…

d4f2792

…ts/gentropy into ds_3238_update_ETL_DAG

DSuveges commented Mar 14, 2024

View reviewed changes

DSuveges requested a review from ireneisdoomed March 14, 2024 10:32

DSuveges merged commit 4dcecc3 into dev Mar 14, 2024
4 checks passed

DSuveges deleted the ds_3238_update_ETL_DAG branch March 14, 2024 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dag): add data transfer task group for release process #528

feat(dag): add data transfer task group for release process #528

DSuveges commented Mar 7, 2024 •

edited

Loading

ireneisdoomed left a comment

ireneisdoomed Mar 12, 2024

ireneisdoomed Mar 12, 2024

DSuveges Mar 13, 2024 •

edited

Loading

ireneisdoomed Mar 12, 2024

DSuveges Mar 13, 2024

d0choa Mar 13, 2024

ireneisdoomed Mar 12, 2024

DSuveges Mar 13, 2024

ireneisdoomed left a comment

ireneisdoomed Mar 13, 2024

DSuveges Mar 13, 2024

ireneisdoomed Mar 13, 2024

DSuveges Mar 14, 2024 •

edited

Loading

ireneisdoomed Mar 14, 2024

ireneisdoomed commented Mar 14, 2024

DSuveges commented Mar 14, 2024



		# Test if release folder exists:
		def test_release_folder_exists() -> bool:

feat(dag): add data transfer task group for release process #528

feat(dag): add data transfer task group for release process #528

Conversation

DSuveges commented Mar 7, 2024 • edited Loading

✨ Context

🛠 What does this PR implement

🚦 Before submitting

ireneisdoomed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DSuveges Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ireneisdoomed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DSuveges Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ireneisdoomed commented Mar 14, 2024

DSuveges commented Mar 14, 2024

DSuveges commented Mar 7, 2024 •

edited

Loading

DSuveges Mar 13, 2024 •

edited

Loading

DSuveges Mar 14, 2024 •

edited

Loading