From eefc86de63029d7a29ddc2edad6a0e499dedae16 Mon Sep 17 00:00:00 2001 From: Dan King Date: Thu, 11 Jan 2024 12:21:11 -0500 Subject: [PATCH 1/6] [httpx][Makefile][copier] add --timeout to copier; use in Makefile We have long had trouble with copying on flaky Internet connections. AFAIK, this is due to our very aggressive timeouts. This change permits the CLI to override the default timeout in `hailtop.aiotools.copy`. Setting this back to 600s should work on most flaky Internet connections. It works on my particularly bad home WiFi. The upload-docs target was old and broken so I deleted it. I did not change the upload-artifacts target because that is Google Cloud specific anyway. --- hail/Makefile | 21 +++++++-------------- hail/python/hailtop/aiotools/copy.py | 19 ++++++++++++++++++- hail/python/hailtop/httpx.py | 12 +++++++++++- hail/scripts/upload_qob_jar.sh | 6 +++++- 4 files changed, 41 insertions(+), 17 deletions(-) diff --git a/hail/Makefile b/hail/Makefile index 18eaf173d9fb..8c2a4e0de0da 100644 --- a/hail/Makefile +++ b/hail/Makefile @@ -349,13 +349,13 @@ upload-artifacts: $(WHEEL) # > rm upload-remote-test-resources upload-remote-test-resources: $(shell git ls-files src/test/resources) upload-remote-test-resources: $(shell git ls-files python/hail/docs/data) - gcloud storage cp -r src/test/resources/\* $(CLOUD_HAIL_TEST_RESOURCES_DIR) - gcloud storage cp -r python/hail/docs/data/\* $(CLOUD_HAIL_DOCTEST_DATA_DIR) - # # In Azure, use the following instead of gcloud storage cp - # python3 -m hailtop.aiotools.copy -vvv 'null' '[\ - # {"from":"src/test/resources","to":"$(CLOUD_HAIL_TEST_RESOURCES_DIR)"},\ - # {"from":"python/hail/docs/data","to":"$(CLOUD_HAIL_DOCTEST_DATA_DIR)"}\ - # ]' + # # If hailtop.aiotools.copy gives you trouble: + # gcloud storage cp -r src/test/resources/\* $(CLOUD_HAIL_TEST_RESOURCES_DIR) + # gcloud storage cp -r python/hail/docs/data/\* $(CLOUD_HAIL_DOCTEST_DATA_DIR) + python3 -m hailtop.aiotools.copy -vvv 'null' '[\ + {"from":"src/test/resources","to":"$(CLOUD_HAIL_TEST_RESOURCES_DIR)"},\ + {"from":"python/hail/docs/data","to":"$(CLOUD_HAIL_DOCTEST_DATA_DIR)"}\ + ]' --timeout 600 touch $@ # NOTE: 1-day expiration of the test bucket means that this @@ -506,13 +506,6 @@ hail-docs-do-not-render-notebooks: $(PYTHON_VERSION_INFO) python/hail/docs/chang -exec $(SED_INPLACE) -e "s/\.css/\.css\?v\=$(HAIL_CACHE_VERSION)/" {} +; @echo Built docs: file://$(HAIL_HAIL_DIR)/build/www/docs/$(HAIL_MAJOR_MINOR_VERSION)/index.html -.PHONY: upload-docs -upload-docs: hail-docs batch-docs - cd build && tar czf www.tar.gz www - gcloud storage cp build/www.tar.gz $(docs_location) - gcloud storage objects update -r $(docs_location) --temporary-hold - gcloud storage objects update -r $(docs_location) --add-acl-grant=entity=AllUsers,role=READER - .PHONY: test test: pytest jvm-test doctest diff --git a/hail/python/hailtop/aiotools/copy.py b/hail/python/hailtop/aiotools/copy.py index 51514156abd6..8cc1b705d863 100644 --- a/hail/python/hailtop/aiotools/copy.py +++ b/hail/python/hailtop/aiotools/copy.py @@ -169,6 +169,7 @@ async def main() -> None: parser.add_argument( '-v', '--verbose', action='store_const', const=True, default=False, help='show logging information' ) + parser.add_argument('--timeout', type=str, default=None, help='show logging information') args = parser.parse_args() if args.verbose: @@ -179,11 +180,27 @@ async def main() -> None: if args.files is None or args.files == '-': args.files = sys.stdin.read() files = json.loads(args.files) - gcs_kwargs = {'gcs_requester_pays_configuration': requester_pays_project} + + timeout = args.timeout + if timeout: + timeout = float(timeout) + print(timeout) + gcs_kwargs = { + 'gcs_requester_pays_configuration': requester_pays_project, + 'timeout': timeout, + } + azure_kwargs = { + 'timeout': timeout, + } + s3_kwargs = { + 'timeout': timeout, + } await copy_from_dict( max_simultaneous_transfers=args.max_simultaneous_transfers, gcs_kwargs=gcs_kwargs, + azure_kwargs=azure_kwargs, + s3_kwargs=s3_kwargs, files=files, verbose=args.verbose, ) diff --git a/hail/python/hailtop/httpx.py b/hail/python/hailtop/httpx.py index 41a9dea053a5..a40a6b5cf8b1 100644 --- a/hail/python/hailtop/httpx.py +++ b/hail/python/hailtop/httpx.py @@ -83,7 +83,11 @@ async def __aexit__( class ClientSession: def __init__( - self, *args, raise_for_status: bool = True, timeout: Union[aiohttp.ClientTimeout, float, None] = None, **kwargs + self, + *args, + raise_for_status: bool = True, + timeout: Union[aiohttp.ClientTimeout, float, int, None] = None, + **kwargs, ): location = get_deploy_config().location() if location == 'external': @@ -99,6 +103,8 @@ def __init__( if timeout is None: timeout = aiohttp.ClientTimeout(total=5) + if isinstance(timeout, (float, int)): + timeout = aiohttp.ClientTimeout(total=timeout) self.loop = asyncio.get_running_loop() self.raise_for_status = raise_for_status @@ -115,6 +121,10 @@ def request( ) raise_for_status = kwargs.pop('raise_for_status', self.raise_for_status) + timeout = kwargs.get('timeout') + if timeout and isinstance(timeout, (float, int)): + kwargs['timeout'] = aiohttp.ClientTimeout(total=timeout) + async def request_and_raise_for_status(): json_data = kwargs.pop('json', None) if json_data is not None: diff --git a/hail/scripts/upload_qob_jar.sh b/hail/scripts/upload_qob_jar.sh index 7aeb20b6c771..1ca4f1cc7e5f 100644 --- a/hail/scripts/upload_qob_jar.sh +++ b/hail/scripts/upload_qob_jar.sh @@ -22,5 +22,9 @@ else JAR_LOCATION="${TEST_STORAGE_URI}/${NAMESPACE}/jars/${TOKEN}/${REVISION}.jar" fi -python3 -m hailtop.aiotools.copy -vvv 'null' '[{"from":"'${SHADOW_JAR}'", "to":"'${JAR_LOCATION}'"}]' +python3 -m hailtop.aiotools.copy \ + -vvv \ + 'null' \ + '[{"from":"'${SHADOW_JAR}'", "to":"'${JAR_LOCATION}'"}]' \ + --timeout 600 echo ${JAR_LOCATION} > ${PATH_FILE} From 256a5cc29384ff1cca69148e659bdfce0c00fa26 Mon Sep 17 00:00:00 2001 From: Dan King Date: Fri, 12 Jan 2024 13:17:15 -0500 Subject: [PATCH 2/6] fix azure and aws handling of timeout --- hail/python/hailtop/aiocloud/aioaws/fs.py | 36 ++++++++++++++++++++- hail/python/hailtop/aiocloud/aioazure/fs.py | 23 +++++++++++-- 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/hail/python/hailtop/aiocloud/aioaws/fs.py b/hail/python/hailtop/aiocloud/aioaws/fs.py index 62623f66e2c7..4605e943a918 100644 --- a/hail/python/hailtop/aiocloud/aioaws/fs.py +++ b/hail/python/hailtop/aiocloud/aioaws/fs.py @@ -1,5 +1,19 @@ -from typing import Any, AsyncIterator, BinaryIO, cast, AsyncContextManager, Dict, List, Optional, Set, Tuple, Type +from typing import ( + Any, + AsyncIterator, + BinaryIO, + cast, + AsyncContextManager, + Dict, + List, + Optional, + Set, + Tuple, + Type, + Union, +) from types import TracebackType +import aiohttp import sys from concurrent.futures import ThreadPoolExecutor import os.path @@ -329,12 +343,32 @@ def __init__( max_workers: Optional[int] = None, *, max_pool_connections: int = 10, + timeout: Optional[Union[int, float, aiohttp.ClientTimeout]], ): if not thread_pool: thread_pool = ThreadPoolExecutor(max_workers=max_workers) self._thread_pool = thread_pool + + kwargs = {} + if isinstance(timeout, aiohttp.ClientTimeout): + if timeout.sock_read: + kwargs['read_timeout'] = timeout.sock_read + elif timeout.total: + kwargs['read_timeout'] = timeout.total + + if timeout.sock_connect: + kwargs['connect_timeout'] = timeout.sock_connect + elif timeout.connect: + kwargs['connect_timeout'] = timeout.connect + elif timeout.total: + kwargs['connect_timeout'] = timeout.total + elif isinstance(timeout, (int, float)): + kwargs['read_timeout'] = timeout + kwargs['connect_timeout'] = timeout + config = botocore.config.Config( max_pool_connections=max_pool_connections, + **kwargs, ) self._s3 = boto3.client('s3', config=config) diff --git a/hail/python/hailtop/aiocloud/aioazure/fs.py b/hail/python/hailtop/aiocloud/aioazure/fs.py index ad04934ea3b2..ef3d4fd649e4 100644 --- a/hail/python/hailtop/aiocloud/aioazure/fs.py +++ b/hail/python/hailtop/aiocloud/aioazure/fs.py @@ -1,6 +1,7 @@ from typing import Any, AsyncContextManager, AsyncIterator, Dict, List, Optional, Set, Tuple, Type, Union from types import TracebackType +import aiohttp import abc import re import asyncio @@ -364,7 +365,13 @@ async def wrapped(self: 'AzureAsyncFS', url, *args, **kwargs): class AzureAsyncFS(AsyncFS): PATH_REGEX = re.compile('/(?P[^/]+)(?P.*)') - def __init__(self, *, credential_file: Optional[str] = None, credentials: Optional[AzureCredentials] = None): + def __init__( + self, + *, + credential_file: Optional[str] = None, + credentials: Optional[AzureCredentials] = None, + timeout: Optional[Union[int, float, aiohttp.ClientTimeout]], + ): if credentials is None: scopes = ['https://storage.azure.com/.default'] if credential_file is not None: @@ -374,6 +381,16 @@ def __init__(self, *, credential_file: Optional[str] = None, credentials: Option elif credential_file is not None: raise ValueError('credential and credential_file cannot both be defined') + if isinstance(timeout, aiohttp.ClientTimeout): + self.read_timeout = timeout.sock_read or timeout.total or 5 + self.connection_timeout = timeout.sock_connect or timeout.connect or timeout.total or 5 + elif isinstance(timeout, (int, float)): + self.read_timeout = timeout + self.connection_timeout = timeout + else: + self.read_timeout = 5 + self.connection_timeout = 5 + self._credential = credentials.credential self._blob_service_clients: Dict[Tuple[str, str, Union[AzureCredentials, str, None]], BlobServiceClient] = {} @@ -474,8 +491,8 @@ def get_blob_service_client(self, account: str, container: str, token: Optional[ self._blob_service_clients[k] = BlobServiceClient( f'https://{account}.blob.core.windows.net', credential=credential, # type: ignore - connection_timeout=5, - read_timeout=5, + connection_timeout=self.connection_timeout, + read_timeout=self.read_timeout, ) return self._blob_service_clients[k] From ca41242c4b2135146bea7470cd2fdffc1056908b Mon Sep 17 00:00:00 2001 From: Dan King Date: Fri, 12 Jan 2024 13:44:44 -0500 Subject: [PATCH 3/6] cleanup dependencies, ensure hailtop is installed --- hail/Makefile | 8 +++----- hail/python/hailtop/aiotools/copy.py | 1 - 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/hail/Makefile b/hail/Makefile index 8c2a4e0de0da..56d3ea0030a9 100644 --- a/hail/Makefile +++ b/hail/Makefile @@ -181,8 +181,7 @@ $(FAST_PYTHON_JAR_EXTRA_CLASSPATH): build.gradle python-jar: $(PYTHON_JAR) .PHONY: pytest -pytest: $(PYTHON_VERSION_INFO) $(INIT_SCRIPTS) -pytest: python/README.md $(FAST_PYTHON_JAR) $(FAST_PYTHON_JAR_EXTRA_CLASSPATH) +pytest: install-editable cd python && \ $(HAIL_PYTHON3) -m pytest \ -Werror:::hail -Werror:::hailtop -Werror::ResourceWarning \ @@ -202,9 +201,7 @@ pytest: python/README.md $(FAST_PYTHON_JAR) $(FAST_PYTHON_JAR_EXTRA_CLASSPATH) # NOTE: Look at upload-remote-test-resources target if test resources are missing .PHONY: pytest-inter-cloud -pytest-inter-cloud: $(PYTHON_VERSION_INFO) $(INIT_SCRIPTS) -pytest-inter-cloud: python/README.md $(FAST_PYTHON_JAR) $(FAST_PYTHON_JAR_EXTRA_CLASSPATH) -pytest-inter-cloud: upload-remote-test-resources +pytest-inter-cloud: upload-remote-test-resources install-editable cd python && \ HAIL_TEST_STORAGE_URI=$(TEST_STORAGE_URI) \ HAIL_TEST_GCS_BUCKET=$(HAIL_TEST_GCS_BUCKET) \ @@ -347,6 +344,7 @@ upload-artifacts: $(WHEEL) # NOTE: 1-day expiration of the test bucket means that this # target must be run at least once a day. To trigger this target to re-run, # > rm upload-remote-test-resources +upload-remote-test-resources: install-editable upload-remote-test-resources: $(shell git ls-files src/test/resources) upload-remote-test-resources: $(shell git ls-files python/hail/docs/data) # # If hailtop.aiotools.copy gives you trouble: diff --git a/hail/python/hailtop/aiotools/copy.py b/hail/python/hailtop/aiotools/copy.py index 8cc1b705d863..0e1d5d30b649 100644 --- a/hail/python/hailtop/aiotools/copy.py +++ b/hail/python/hailtop/aiotools/copy.py @@ -184,7 +184,6 @@ async def main() -> None: timeout = args.timeout if timeout: timeout = float(timeout) - print(timeout) gcs_kwargs = { 'gcs_requester_pays_configuration': requester_pays_project, 'timeout': timeout, From 3d2445d9e25bd89bf297928e184c3002cd9e7e2c Mon Sep 17 00:00:00 2001 From: Dan King Date: Fri, 12 Jan 2024 15:15:53 -0500 Subject: [PATCH 4/6] timeout is not required --- hail/python/hailtop/aiocloud/aioaws/fs.py | 2 +- hail/python/hailtop/aiocloud/aioazure/fs.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hail/python/hailtop/aiocloud/aioaws/fs.py b/hail/python/hailtop/aiocloud/aioaws/fs.py index 4605e943a918..64a5a90d3fe9 100644 --- a/hail/python/hailtop/aiocloud/aioaws/fs.py +++ b/hail/python/hailtop/aiocloud/aioaws/fs.py @@ -343,7 +343,7 @@ def __init__( max_workers: Optional[int] = None, *, max_pool_connections: int = 10, - timeout: Optional[Union[int, float, aiohttp.ClientTimeout]], + timeout: Optional[Union[int, float, aiohttp.ClientTimeout]] = None, ): if not thread_pool: thread_pool = ThreadPoolExecutor(max_workers=max_workers) diff --git a/hail/python/hailtop/aiocloud/aioazure/fs.py b/hail/python/hailtop/aiocloud/aioazure/fs.py index ef3d4fd649e4..4c7a05bbc241 100644 --- a/hail/python/hailtop/aiocloud/aioazure/fs.py +++ b/hail/python/hailtop/aiocloud/aioazure/fs.py @@ -370,7 +370,7 @@ def __init__( *, credential_file: Optional[str] = None, credentials: Optional[AzureCredentials] = None, - timeout: Optional[Union[int, float, aiohttp.ClientTimeout]], + timeout: Optional[Union[int, float, aiohttp.ClientTimeout]] = None, ): if credentials is None: scopes = ['https://storage.azure.com/.default'] From c440355251ca3974038bc30d29ae689b621db813 Mon Sep 17 00:00:00 2001 From: Dan King Date: Tue, 16 Jan 2024 14:52:19 -0500 Subject: [PATCH 5/6] demand POSIX compatibility. --- hail/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/hail/Makefile b/hail/Makefile index 56d3ea0030a9..15e74fe1085b 100644 --- a/hail/Makefile +++ b/hail/Makefile @@ -1,4 +1,5 @@ .PHONY: jars clean +.POSIX: # https://github.com/hail-is/hail/pull/14138#issuecomment-1894411324 HAIL_HAIL_DIR := $(shell dirname $(realpath $(firstword $(MAKEFILE_LIST)))) From 630ffd3b41e29e31ec0a1d6d40b7b435e32e6d23 Mon Sep 17 00:00:00 2001 From: Dan King Date: Tue, 16 Jan 2024 15:08:31 -0500 Subject: [PATCH 6/6] actually fix the JSON escaping --- hail/Makefile | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/hail/Makefile b/hail/Makefile index 15e74fe1085b..c27b5deebd94 100644 --- a/hail/Makefile +++ b/hail/Makefile @@ -1,5 +1,4 @@ .PHONY: jars clean -.POSIX: # https://github.com/hail-is/hail/pull/14138#issuecomment-1894411324 HAIL_HAIL_DIR := $(shell dirname $(realpath $(firstword $(MAKEFILE_LIST)))) @@ -351,10 +350,14 @@ upload-remote-test-resources: $(shell git ls-files python/hail/docs/data) # # If hailtop.aiotools.copy gives you trouble: # gcloud storage cp -r src/test/resources/\* $(CLOUD_HAIL_TEST_RESOURCES_DIR) # gcloud storage cp -r python/hail/docs/data/\* $(CLOUD_HAIL_DOCTEST_DATA_DIR) - python3 -m hailtop.aiotools.copy -vvv 'null' '[\ - {"from":"src/test/resources","to":"$(CLOUD_HAIL_TEST_RESOURCES_DIR)"},\ - {"from":"python/hail/docs/data","to":"$(CLOUD_HAIL_DOCTEST_DATA_DIR)"}\ - ]' --timeout 600 + python3 -m hailtop.aiotools.copy \ + -vvv \ + 'null' \ + "[\ + {\"from\":\"src/test/resources\",\"to\":\"$(CLOUD_HAIL_TEST_RESOURCES_DIR)\"}, \ + {\"from\":\"python/hail/docs/data\",\"to\":\"$(CLOUD_HAIL_DOCTEST_DATA_DIR)\"} \ + ]" \ + --timeout 600 touch $@ # NOTE: 1-day expiration of the test bucket means that this