Parquet to BigQuery import for GCP-backed AnVIL snapshots (#6355) #6392
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6392 +/- ##
===========================================
- Coverage 85.38% 85.12% -0.26%
===========================================
Files 155 155
Lines 20754 20823 +69
===========================================
+ Hits 17720 17725 +5
- Misses 3034 3098 +64 ☔ View full report in Codecov by Sentry. |
nadove-ucsc
added
the
reqs
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/6355-parquet-bigquery-import-gcp-anvil-snapshots
branch
2 times, most recently
from
ccd127d
to
09c7a41
Compare
dsotirho-ucsc
requested changes
Jul 12, 2024
scripts/verify_tdr_sources.py
Outdated
| 'Actual Google project of TDR source differs from configured one', | ||
| source.project, source_spec.project) | ||
| source.project, source_spec.project, config.google_project()) |
There was a problem hiding this comment.
Suggested change
| source.project, source_spec.project, config.google_project()) | |
| source_spec.project, source.project, config.google_project()) |
to match the order of these variables in the condition part of the require()
There was a problem hiding this comment.
I decided on a different approach here to make the diff clearer and hopefully make the intent more apparent.
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/6355-parquet-bigquery-import-gcp-anvil-snapshots
branch
from
09c7a41
to
0d1c70f
Compare
|
I also moved some changes from the first commit to the 2nd, hence the other fixup. |
hannes-ucsc
requested changes
Jul 22, 2024
Makefile
Outdated
Comment on lines
105
to
106
| python $(project_root)/scripts/reindex.py --import --sources "tdr:${GOOGLE_PROJECT}:snapshot/*" | ||
| python $(project_root)/scripts/verify_tdr_sources.py |
There was a problem hiding this comment.
I think these two commands should be extracted to a separate make target called import, and a corresponding GitLab job. We also need to think about sandbox and personal deployments that typically share the sources with a main deployment. I think #6426 will help with this which is why I've added it as a blocker of #6355.
scripts/download_tdr_parquet.py
Outdated
| @@ -0,0 +1,103 @@ | |||
| """ | |||
| Export parquet files from TDR and download them to local storage. | |||
There was a problem hiding this comment.
Suggested change
| Export parquet files from TDR and download them to local storage. | |
| Export Parquet files from TDR and download them to local storage. |
Here and in elsewhere in documentation.
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/6355-parquet-bigquery-import-gcp-anvil-snapshots
branch
from
0d1c70f
to
b3d454f
Compare
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/6355-parquet-bigquery-import-gcp-anvil-snapshots
branch
2 times, most recently
from
6c01bba
to
2168ed5
Compare
nadove-ucsc
added
the
chained
nadove-ucsc
changed the base branch from
develop
to
issues/nadove-ucsc/6426-cleanup-generalize-tdr-source-spec
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/6355-parquet-bigquery-import-gcp-anvil-snapshots
branch
from
2168ed5
to
aae4509
Compare
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/6426-cleanup-generalize-tdr-source-spec
branch
from
4589e62
to
1a0a33a
Compare
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/6355-parquet-bigquery-import-gcp-anvil-snapshots
branch
from
aae4509
to
5195059
Compare
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/6426-cleanup-generalize-tdr-source-spec
branch
from
192d97a
to
a5f7692
Compare
achave11-ucsc
force-pushed
the
issues/nadove-ucsc/6426-cleanup-generalize-tdr-source-spec
branch
from
6e7481e
to
be2dfe3
Compare
achave11-ucsc
changed the base branch from
issues/nadove-ucsc/6426-cleanup-generalize-tdr-source-spec
to
develop
achave11-ucsc
removed
the
chained
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/6355-parquet-bigquery-import-gcp-anvil-snapshots
branch
4 times, most recently
from
084a127
to
14df96c
Compare
hannes-ucsc
requested changes
Aug 30, 2024
There was a problem hiding this comment.
Somethings amiss here. It appears that this adds code that isn't actually exercised because none of the sources are configured to use Parquet. We should switch one source in dev, sandbox,anvildev and anvilbox to use Parquet. Lets also discuss in PL what to do about personal deployments.
Subject: [PATCH] make fo
---
Index: src/azul/terra.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/azul/terra.py b/src/azul/terra.py
--- a/src/azul/terra.py (revision 14df96cd41cd529093df1520abd73d725b44aa28)
+++ b/src/azul/terra.py (date 1724981432852)
@@ -541,6 +541,7 @@
endpoint: furl,
response: urllib3.HTTPResponse
) -> MutableJSON:
+ # REVIEW: Short comment here explaining when we'd expect a 202
if response.status in (200, 202):
return json.loads(response.data)
# FIXME: Azul sometimes conflates 401 and 403
@@ -661,8 +662,8 @@
def create_dataset(self, dataset_name: str):
"""
- Create a BigQuery dataset in the project and region configured for the
- current deployment.
+ Create a BigQuery dataset in the GCP project associated with the current
+ credentials and the GCP region configure for the current deployment.
:param dataset_name: Unqualified name of the dataset to create.
`google.cloud.exceptions.Conflict` will be raised
@@ -670,10 +671,23 @@
"""
bigquery = self._bigquery(self.credentials.project_id)
ref = DatasetReference(bigquery.project, dataset_name)
+ # We get a false warning from PyCharm here, probably because of
+ #
+ # https://youtrack.jetbrains.com/issue/PY-23400/regression-PEP484-type-annotations-in-docstrings-nearly-completely-broken
+ #
+ # Google uses the docstring syntax to annotate types in its BQ client.
+ #
+ # noinspection PyTypeChecker
dataset = Dataset(ref)
+ # REVIEW: This changes the meaning of AZUL_TDR_SOURCE_LOCATION somewhat.
+ # While I don't think we need to introduce a new variable, we
+ # should document the new semantics so that someone modifying
+ # it is aware of the implications.
dataset.location = config.tdr_source_location
log.info('Creating BigQuery dataset %r in region %r',
dataset.dataset_id, dataset.location)
+ # REVIEW: This method returns something. Let's assert key aspects of the
+ # return value.
bigquery.create_dataset(dataset)
def create_table(self,
@@ -692,8 +706,11 @@
:param table_name: Unqualified name of the new table
+ REVIEW: Technically gs://… is a URI. If "URL" is TDR lingo I'm happy to
+ adopt it, otherwise we should use "URI".
+
:param import_urls: URLs of Parquet file(s) to populate the table. These
- must be `gs://` URLS and the GCS bucket's region
+ must be `gs://` URLs and the GCS bucket's region
must be compatible with the target dataset's. See
https://cloud.google.com/bigquery/docs/loading-data-cloud-storage-parquet#limitations
@@ -705,7 +722,7 @@
https://cloud.google.com/bigquery/docs/clustered-tables
"""
for url in import_urls:
- require(url.scheme == 'gs', url)
+ require(url.scheme == 'gs', 'Expected gs:// URI', url)
table_id = f'{dataset_name}.{table_name}'
bigquery = self._bigquery(self.credentials.project_id)
write_disposition = (
@@ -716,25 +733,26 @@
clustering_fields=clustering_fields,
source_format=SourceFormat.PARQUET,
# Avoids convoluted data types for array fields
+ # REVIEW: Please elaborate
parquet_options=ParquetOptions.from_api_repr(dict(enable_list_inference=True))
)
- log.info('Creating BigQuery table %r',
- f'{bigquery.project}.{dataset_name}.{table_name}')
+ table_ref = f'{bigquery.project}.{table_id}'
+ log.info('Creating BigQuery table %r', table_ref)
load_job = bigquery.load_table_from_uri(source_uris=list(map(str, import_urls)),
destination=table_id,
job_config=job_config)
load_job.result()
- log.info('Table created successfully')
+ log.info('Table %r created successfully', table_ref)
def export_parquet_urls(self,
snapshot_id: str
) -> Optional[dict[str, list[mutable_furl]]]:
"""
Obtain URLs of Parquet files for the data tables of the specified
- snapshot. This is an time-consuming operation that usually takes on the
- order of 1 minute to complete.
+ snapshot. This is a time-consuming operation that usually takes on the
+ order of one minute to complete.
- :param snapshot_id: The UUID of the snapshot.
+ :param snapshot_id: The UUID of the snapshot
:return: A mapping of table names to lists of Parquet file download
URLs, or `None` if if no Parquet downloads are available for
@@ -744,6 +762,8 @@
url = self._repository_endpoint('snapshots', snapshot_id, 'export')
# Required for Azure-backed snapshots
url.args.add('validatePrimaryKeyUniqueness', False)
+ # REVIEW: We should apply a timeout here. I suggest five times the
+ # longest observed duration
while True:
response = self._request('GET', url)
response_body = self._check_response(url, response)
@@ -752,6 +772,8 @@
if jobs_status == 'running':
url = self._repository_endpoint('jobs', job_id)
log.info('Waiting for job %r ...', job_id)
+ # REVIEW: What's this choice of two seconds based on? It seems
+ # rather short considering everything about TDR's robustness.
time.sleep(2)
elif jobs_status == 'succeeded':
break
Index: .gitlab-ci.yml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
--- a/.gitlab-ci.yml (revision 14df96cd41cd529093df1520abd73d725b44aa28)
+++ b/.gitlab-ci.yml (date 1724981655861)
@@ -100,6 +100,8 @@
import:
extends: .base_on_push
stage: deploy
+ # REVIEW: Assume all sources need to be imported, estimate the expected
+ # running time and double that. Remove the comment.
timeout: 5m # probably needs to be extended
needs:
- build_image
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/6355-parquet-bigquery-import-gcp-anvil-snapshots
branch
2 times, most recently
from
2b088c8
to
d7ebee7
Compare
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/6355-parquet-bigquery-import-gcp-anvil-snapshots
branch
from
d7ebee7
to
65fc638
Compare
|
@hannes-ucsc: "Regarding personal deployments, the import should only be performed on shared deployments. By the time developers upgrade their personal deployments to mirror the respective sandbox, the GitLab build for that sandbox will have already imported the snapshot. If someone really needs to import a snapshot for a personal deployment, they can temporarily enable the import for personal deployments." |
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/6355-parquet-bigquery-import-gcp-anvil-snapshots
branch
from
65fc638
to
78545b6
Compare
hannes-ucsc
requested changes
Sep 23, 2024
| import: | ||
| extends: .base_on_push | ||
| stage: deploy | ||
| # The 1000G snapshot on `anvildev` takes about 3.5 minutes to import. There |
There was a problem hiding this comment.
I'm not sure assuming that every snapshot is as big as 1000G leads to practical timeout.
A timeout is a heuristic defense against hung workloads, i.e., workloads that stop making significant progress. We don't want to constantly update the timeout, we don't want it to prematurely kill workloads that are progressing at the average rate, and we don't want the workload to be in the hung state for > 80% of it's running time. A 5min timeout goes against the first rule, a 30h timeout goes against the last.
| @@ -73,7 +73,7 @@ def mkdict(previous_catalog: dict[str, str], | |||
|
|
|||
|
|
|||
| anvil_sources = mkdict({}, 3, mkdelta([ | |||
| mksrc('bigquery', 'datarepo-dev-e53e74aa', 'ANVIL_1000G_2019_Dev_20230609_ANV5_202306121732', 6804), | |||
| mksrc('parquet', 'platform-anvil-dev', 'ANVIL_1000G_2019_Dev_20230609_ANV5_202306121732', 6804), | |||
There was a problem hiding this comment.
The source spec should state where the source is, not where it will be when it is imported. The logic should be to import every parquet source. So I think this should read
Suggested change
| mksrc('parquet', 'platform-anvil-dev', 'ANVIL_1000G_2019_Dev_20230609_ANV5_202306121732', 6804), | |
| mksrc('bigquery', 'platform-anvil-dev', 'ANVIL_1000G_2019_Dev_20230609_ANV5_202306121732', 6804), |
| @@ -64,7 +64,7 @@ def mkdict(previous_catalog: dict[str, str], | |||
|
|
|||
|
|
|||
| anvil_sources = mkdict({}, 3, mkdelta([ | |||
| mksrc('bigquery', 'datarepo-dev-e53e74aa', 'ANVIL_1000G_2019_Dev_20230609_ANV5_202306121732', 6804), | |||
| mksrc('parquet', 'platform-anvil-dev', 'ANVIL_1000G_2019_Dev_20230609_ANV5_202306121732', 6804), | |||
There was a problem hiding this comment.
Suggested change
| mksrc('parquet', 'platform-anvil-dev', 'ANVIL_1000G_2019_Dev_20230609_ANV5_202306121732', 6804), | |
| mksrc('parquet', 'datarepo-dev-e53e74aa', 'ANVIL_1000G_2019_Dev_20230609_ANV5_202306121732', 6804), |
| endef | ||
|
|
||
| $(eval $(call deploy,)) | ||
| $(eval $(call deploy,auto_)) | ||
|
|
||
| .PHONY: import | ||
| import: check_python | ||
| python $(project_root)/scripts/reindex.py --import --sources "tdr:parquet:gcp:${GOOGLE_PROJECT}:*" |
There was a problem hiding this comment.
Suggested change
| python $(project_root)/scripts/reindex.py --import --sources "tdr:parquet:gcp:${GOOGLE_PROJECT}:*" | |
| python $(project_root)/scripts/reindex.py --import --sources "tdr:parquet:gcp:*" |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Connected issues: #6355
Checklist
Author
developissues/<GitHub handle of author>/<issue#>-<slug>1 when the issue title describes a problem, the corresponding PR
title is
Fix:followed by the issue titleAuthor (partiality)
ptag to titles of partial commitspartialor completely resolves all connected issuespartiallabelAuthor (chains)
baseor this PR is not chained to another PRchainedor is not chained to another PRAuthor (reindex, API changes)
rtag to commit title or the changes introduced by this PR will not require reindexing of any deploymentreindex:devor the changes introduced by it will not require reindexing ofdevreindex:anvildevor the changes introduced by it will not require reindexing ofanvildevreindex:anvilprodor the changes introduced by it will not require reindexing ofanvilprodreindex:prodor the changes introduced by it will not require reindexing ofprodreindex:partialand its description documents the specific reindexing procedure fordev,anvildev,anvilprodandprodor requires a full reindex or carries none of the labelsreindex:dev,reindex:anvildev,reindex:anvilprodandreindex:prodAPIor this PR does not modify a REST APIa(A) tag to commit title for backwards (in)compatible changes or this PR does not modify a REST APIapp.pyor this PR does not modify a REST APIAuthor (upgrading deployments)
make image_manifests.jsonand committed the resulting changes or this PR does not modifyazul_docker_images, or any other variables referenced in the definition of that variableutag to commit title or this PR does not require upgrading deploymentsupgradeor does not require upgrading deploymentsdeploy:sharedor does not modifyimage_manifests.json, and does not require deploying thesharedcomponent for any other reasondeploy:gitlabor does not require deploying thegitlabcomponentdeploy:runneror does not require deploying therunnerimageAuthor (hotfixes)
Ftag to main commit title or this PR does not include permanent fix for a temporary hotfixanvilprodandprod) have temporary hotfixes for any of the issues connected to this PRAuthor (before every review)
develop, squashed old fixupsmake requirements_updateor this PR does not modifyrequirements*.txt,common.mk,MakefileandDockerfileRtag to commit title or this PR does not modifyrequirements*.txtreqsor does not modifyrequirements*.txtmake integration_testpasses in personal deployment or this PR does not modify functionality that could affect the IT outcomePeer reviewer (after approval)
System administrator (after approval)
demoorno demono demono sandboxN reviewslabel is accurateOperator (before pushing merge the commit)
reindex:…labels andrcommit title tagno demodevelop_select dev.shared && CI_COMMIT_REF_NAME=develop make -C terraform/shared apply_keep_unusedor this PR is not labeleddeploy:shared_select dev.gitlab && CI_COMMIT_REF_NAME=develop make -C terraform/gitlab applyor this PR is not labeleddeploy:gitlab_select anvildev.shared && CI_COMMIT_REF_NAME=develop make -C terraform/shared apply_keep_unusedor this PR is not labeleddeploy:shared_select anvildev.gitlab && CI_COMMIT_REF_NAME=develop make -C terraform/gitlab applyor this PR is not labeleddeploy:gitlabdeploy:gitlabdeploy:gitlabSystem administrator
dev.gitlabare complete or this PR is not labeleddeploy:gitlabanvildev.gitlabare complete or this PR is not labeleddeploy:gitlabOperator (before pushing merge the commit)
_select dev.gitlab && make -C terraform/gitlab/runneror this PR is not labeleddeploy:runner_select anvildev.gitlab && make -C terraform/gitlab/runneror this PR is not labeleddeploy:runnersandboxlabel or PR is labeledno sandboxdevor PR is labeledno sandboxanvildevor PR is labeledno sandboxsandboxdeployment or PR is labeledno sandboxanvilboxdeployment or PR is labeledno sandboxsandboxdeployment or PR is labeledno sandboxanvilboxdeployment or PR is labeledno sandboxsandboxor this PR does not remove catalogs or otherwise causes unreferenced indices indevanvilboxor this PR does not remove catalogs or otherwise causes unreferenced indices inanvildevsandboxor this PR is not labeledreindex:devanvilboxor this PR is not labeledreindex:anvildevsandboxor this PR is not labeledreindex:devanvilboxor this PR is not labeledreindex:anvildevpif the PR is also labeledpartialOperator (chain shortening)
developor this PR is not labeledbasechainedlabel from the blocked PR or this PR is not labeledbasebasebaselabel from this PR or this PR is not labeledbaseOperator (after pushing the merge commit)
devanvildevdevdevanvildevanvildev_select dev.shared && make -C terraform/shared applyor this PR is not labeleddeploy:shared_select anvildev.shared && make -C terraform/shared applyor this PR is not labeleddeploy:shareddevanvildevOperator (reindex)
devor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevOperator
deploy:shared,deploy:gitlab,deploy:runner,API,reindex:partial,reindex:anvilprodandreindex:prodlabels to the next promotion PRs or this PR carries none of these labelsdeploy:shared,deploy:gitlab,deploy:runner,API,reindex:partial,reindex:anvilprodandreindex:prodlabels, from the description of this PR to that of the next promotion PRs or this PR carries none of these labelsShorthand for review comments
Lline is too longWline wrapping is wrongQbad quotesFother formatting problem