diff --git a/.gitignore b/.gitignore index 5be2e55dd4..b11afb7abb 100644 --- a/.gitignore +++ b/.gitignore @@ -22,3 +22,4 @@ tmp/ .coverage* .mypy_cache /contrib/admin/.issue_cache/ +.docker_cache/ \ No newline at end of file diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 7f694ad4a4..1abd2d5885 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -20,6 +20,8 @@ before_script: echo "{\"registry-mirrors\": [\"${DOCKER_HUB_MIRROR}\"], \"insecure-registries\": [\"${DOCKER_HUB_MIRROR##*://}\"]}" | sudo tee /etc/docker/daemon.json export SINGULARITY_DOCKER_HUB_MIRROR="${DOCKER_HUB_MIRROR}" fi + # Restart or start the Docker daemon + - stopdocker || true - startdocker || true - docker info - cat /etc/hosts @@ -29,9 +31,9 @@ before_script: # a service account bearer token for auth and triggers https://github.com/docker/buildx/issues/267 # where buildx can't use a bearer token from a kube config and falls back to anonymous instead # of using the system's service account. - - KUBECONFIG=/dev/null docker buildx create --use --name toilbuilder --platform=linux/amd64,linux/arm64 --node=buildkit-amd64 --driver=kubernetes --driver-opt="nodeselector=kubernetes.io/arch=amd64" - # Dump the builder info, and make sure it exists. - - docker buildx inspect --bootstrap || (echo "Docker builder deployment can't be found in our Kubernetes namespace! Are we on the right Gitlab runner?" && exit 1) + - if [[ "${CI_BUILDKIT_DRIVER}" == "kubernetes" ]] ; then KUBECONFIG=/dev/null docker buildx create --use --name=buildkit --platform=linux/amd64,linux/arm64 --node=buildkit-amd64 --driver=kubernetes --driver-opt="nodeselector=kubernetes.io/arch=amd64" ; else docker buildx create --use --name=container-builder --driver=docker-container ; fi + # Report on the builders, and make sure they exist. + - docker buildx inspect --bootstrap || (echo "Docker builder deployment can't be found! Are we on the right Gitlab runner?" && exit 1) # This will hang if we can't talk to the builder - (echo "y" | docker buildx prune --keep-storage 80G) || true diff --git a/Makefile b/Makefile index 12f6bd5c54..601c6f8a77 100644 --- a/Makefile +++ b/Makefile @@ -170,24 +170,28 @@ pre_pull_docker: for i in $$(seq 1 11); do if [[ $$i == "11" ]] ; then exit 1 ; fi ; docker pull sscaling/mtail && break || sleep 60; done toil_docker: pre_pull_docker docker/Dockerfile + mkdir -p .docker_cache @set -ex \ ; cd docker \ - ; docker buildx build --platform=$(arch) --tag=$(docker_image):$(TOIL_DOCKER_TAG) -f Dockerfile . + ; docker buildx build --platform=$(arch) --tag=$(docker_image):$(TOIL_DOCKER_TAG) --cache-from type=registry,ref=$(docker_image):$(TOIL_DOCKER_MAIN_CACHE_TAG) --cache-from type=registry,ref=$(docker_image):$(TOIL_DOCKER_CACHE_TAG) --cache-from type=local,src=../.docker-cache/toil --cache-to type=local,dest=../.docker-cache/toil -f Dockerfile . prometheus_docker: pre_pull_docker + mkdir -p .docker_cache @set -ex \ ; cd dashboard/prometheus \ - ; docker buildx build --platform=$(arch) --tag=$(prometheus_image):$(TOIL_DOCKER_TAG) -f Dockerfile . + ; docker buildx build --platform=$(arch) --tag=$(prometheus_image):$(TOIL_DOCKER_TAG) --cache-from type=registry,ref=$(prometheus_image):$(TOIL_DOCKER_MAIN_CACHE_TAG) --cache-from type=registry,ref=$(prometheus_image):$(TOIL_DOCKER_CACHE_TAG) --cache-from type=local,src=../../.docker-cache/prometheus --cache-to type=local,dest=../../.docker-cache/prometheus -f Dockerfile . grafana_docker: pre_pull_docker + mkdir -p .docker_cache @set -ex \ ; cd dashboard/grafana \ - ; docker buildx build --platform=$(arch) --tag=$(grafana_image):$(TOIL_DOCKER_TAG) -f Dockerfile . + ; docker buildx build --platform=$(arch) --tag=$(grafana_image):$(TOIL_DOCKER_TAG) --cache-from type=registry,ref=$(grafana_image):$(TOIL_DOCKER_MAIN_CACHE_TAG) --cache-from type=registry,ref=$(grafana_image):$(TOIL_DOCKER_CACHE_TAG) --cache-from type=local,src=../../.docker-cache/grafana --cache-to type=local,dest=../../.docker-cache/grafana -f Dockerfile . mtail_docker: pre_pull_docker + mkdir -p .docker_cache @set -ex \ ; cd dashboard/mtail \ - ; docker buildx build --platform=$(arch) --tag=$(mtail_image):$(TOIL_DOCKER_TAG) -f Dockerfile . + ; docker buildx build --platform=$(arch) --tag=$(mtail_image):$(TOIL_DOCKER_TAG) --cache-from type=registry,ref=$(mtail_image):$(TOIL_DOCKER_MAIN_CACHE_TAG) --cache-from type=registry,ref=$(mtail_image):$(TOIL_DOCKER_CACHE_TAG) --cache-from type=local,src=../../.docker-cache/mtail --cache-to type=local,dest=../../.docker-cache/mtail -f Dockerfile . docker/$(sdist_name): dist/$(sdist_name) cp $< $@ @@ -200,17 +204,42 @@ clean_docker: -docker rmi $(docker_image):$(TOIL_DOCKER_TAG) push_docker: docker - # Weird if logic is so we fail if all the pushes fail - cd docker ; for i in $$(seq 1 6); do if [[ $$i == "6" ]] ; then exit 1 ; fi ; docker buildx build --platform $(arch) --push --tag=$(docker_image):$(TOIL_DOCKER_TAG) -f Dockerfile . && break || sleep 60; done - cd dashboard/prometheus ; for i in $$(seq 1 6); do if [[ $$i == "6" ]] ; then exit 1 ; fi ; docker buildx build --platform $(arch) --push --tag=$(prometheus_image):$(TOIL_DOCKER_TAG) -f Dockerfile . && break || sleep 60; done - cd dashboard/grafana ; for i in $$(seq 1 6); do if [[ $$i == "6" ]] ; then exit 1 ; fi ; docker buildx build --platform $(arch) --push --tag=$(grafana_image):$(TOIL_DOCKER_TAG) -f Dockerfile . && break || sleep 60; done - cd dashboard/mtail ; for i in $$(seq 1 6); do if [[ $$i == "6" ]] ; then exit 1 ; fi ; docker buildx build --platform $(arch) --push --tag=$(mtail_image):$(TOIL_DOCKER_TAG) -f Dockerfile . && break || sleep 60; done + # Weird if logic is so we fail if all the pushes fail. + # We need to build from the local cache to the cache tag and again from the local cache to the real tag. + cd docker ; \ + for i in $$(seq 1 6); do \ + if [[ $$i == "6" ]] ; then exit 1 ; fi ; \ + docker buildx build --platform $(arch) --push --tag=$(docker_image):$(TOIL_DOCKER_CACHE_TAG) --cache-from type=local,src=../.docker-cache/toil --cache-to type=inline -f Dockerfile . && \ + docker buildx build --platform $(arch) --push --tag=$(docker_image):$(TOIL_DOCKER_TAG) --cache-from type=local,src=../.docker-cache/toil -f Dockerfile . && \ + break || sleep 60; \ + done + cd dashboard/prometheus ; \ + for i in $$(seq 1 6); do \ + if [[ $$i == "6" ]] ; then exit 1 ; fi ; \ + docker buildx build --platform $(arch) --push --tag=$(prometheus_image):$(TOIL_DOCKER_CACHE_TAG) --cache-from type=local,src=../../.docker-cache/prometheus --cache-to type=inline -f Dockerfile . && \ + docker buildx build --platform $(arch) --push --tag=$(prometheus_image):$(TOIL_DOCKER_TAG) --cache-from type=local,src=../../.docker-cache/prometheus -f Dockerfile . && \ + break || sleep 60; \ + done + cd dashboard/grafana ; \ + for i in $$(seq 1 6); do \ + if [[ $$i == "6" ]] ; then exit 1 ; fi ; \ + docker buildx build --platform $(arch) --push --tag=$(grafana_image):$(TOIL_DOCKER_CACHE_TAG) --cache-from type=local,src=../../.docker-cache/grafana --cache-to type=inline -f Dockerfile . && \ + docker buildx build --platform $(arch) --push --tag=$(grafana_image):$(TOIL_DOCKER_TAG) --cache-from type=local,src=../../.docker-cache/grafana -f Dockerfile . && \ + break || sleep 60; \ + done + cd dashboard/mtail ; \ + for i in $$(seq 1 6); do \ + if [[ $$i == "6" ]] ; then exit 1 ; fi ; \ + docker buildx build --platform $(arch) --push --tag=$(mtail_image):$(TOIL_DOCKER_CACHE_TAG) --cache-from type=local,src=../../.docker-cache/mtail --cache-to type=inline -f Dockerfile . && \ + docker buildx build --platform $(arch) --push --tag=$(mtail_image):$(TOIL_DOCKER_TAG) --cache-from type=local,src=../../.docker-cache/mtail -f Dockerfile . && \ + break || sleep 60; \ + done load_docker: docker - cd docker ; docker buildx build --platform $(arch) --load --tag=$(docker_image):$(TOIL_DOCKER_TAG) -f Dockerfile . - cd dashboard/prometheus ; docker buildx build --platform $(arch) --load --tag=$(prometheus_image):$(TOIL_DOCKER_TAG) -f Dockerfile . - cd dashboard/grafana ; docker buildx build --platform $(arch) --load --tag=$(grafana_image):$(TOIL_DOCKER_TAG) -f Dockerfile . - cd dashboard/mtail ; docker buildx build --platform $(arch) --load --tag=$(mtail_image):$(TOIL_DOCKER_TAG) -f Dockerfile . + cd docker ; docker buildx build --platform $(arch) --load --tag=$(docker_image):$(TOIL_DOCKER_TAG) --cache-from type=local,src=../.docker-cache/toil -f Dockerfile . + cd dashboard/prometheus ; docker buildx build --platform $(arch) --load --tag=$(prometheus_image):$(TOIL_DOCKER_TAG) --cache-from type=local,src=../../.docker-cache/prometheus -f Dockerfile . + cd dashboard/grafana ; docker buildx build --platform $(arch) --load --tag=$(grafana_image):$(TOIL_DOCKER_TAG) --cache-from type=local,src=../../.docker-cache/grafana -f Dockerfile . + cd dashboard/mtail ; docker buildx build --platform $(arch) --load --tag=$(mtail_image):$(TOIL_DOCKER_TAG) --cache-from type=local,src=../../.docker-cache/mtail -f Dockerfile . else diff --git a/common.mk b/common.mk index 32d00128dd..00ebc208d6 100644 --- a/common.mk +++ b/common.mk @@ -15,6 +15,8 @@ SHELL=bash export TOIL_DOCKER_REGISTRY?=quay.io/ucsc_cgl export TOIL_DOCKER_NAME?=toil export TOIL_DOCKER_TAG?=$(shell python version_template.py dockerTag) +export TOIL_DOCKER_CACHE_TAG?=$(shell python version_template.py cacheTag) +export TOIL_DOCKER_MAIN_CACHE_TAG?=$(shell python version_template.py mainCacheTag) export TOIL_APPLIANCE_SELF?=$(TOIL_DOCKER_REGISTRY)/$(TOIL_DOCKER_NAME):$(TOIL_DOCKER_TAG) # TOIL_CHECK_ENV='' # Determines whether toil refers to the same virtualenv paths it spawned from (across machines) diff --git a/requirements-aws.txt b/requirements-aws.txt index 77e14e7339..1ae51c64f6 100644 --- a/requirements-aws.txt +++ b/requirements-aws.txt @@ -1,3 +1,4 @@ boto>=2.48.0, <3 -boto3-stubs[s3,sdb,iam,sts,boto3]>=1.28.3, <2 -moto>=4.1.11, <5 \ No newline at end of file +boto3-stubs[s3,sdb,iam,sts,boto3]>=1.28.3.post2, <2 +mypy-boto3-iam>=1.28.3.post2, <2 # Need to force .post1 to be replaced +moto>=4.1.11, <5 diff --git a/requirements-google.txt b/requirements-google.txt index f7be85b573..8e405c8a38 100644 --- a/requirements-google.txt +++ b/requirements-google.txt @@ -1,2 +1,3 @@ apache-libcloud>=2.2.1,<3 google-cloud-storage>=2,<=2.8.0 +google-auth>=2.18.1,<3 diff --git a/setup.cfg b/setup.cfg index 40a862e4ae..8b4ba740ca 100644 --- a/setup.cfg +++ b/setup.cfg @@ -18,7 +18,8 @@ markers = docker_cuda encryption fetchable_appliance - google + google-project + google-storage gridengine htcondor integrative diff --git a/src/toil/common.py b/src/toil/common.py index 344515263a..82b418f2b6 100644 --- a/src/toil/common.py +++ b/src/toil/common.py @@ -1266,27 +1266,55 @@ def importFile(self, def import_file(self, src_uri: str, shared_file_name: str, - symlink: bool = True) -> None: ... + symlink: bool = True, + check_existence: bool = True) -> None: ... @overload def import_file(self, src_uri: str, shared_file_name: None = None, - symlink: bool = True) -> FileID: ... + symlink: bool = True, + check_existence: bool = True) -> FileID: ... def import_file(self, src_uri: str, shared_file_name: Optional[str] = None, - symlink: bool = True) -> Optional[FileID]: + symlink: bool = True, + check_existence: bool = True) -> Optional[FileID]: """ Import the file at the given URL into the job store. + By default, returns None if the file does not exist. + + :param check_existence: If true, raise FileNotFoundError if the file + does not exist. If false, return None when the file does not + exist. + See :func:`toil.jobStores.abstractJobStore.AbstractJobStore.importFile` for a full description """ self._assertContextManagerUsed() - src_uri = self.normalize_uri(src_uri, check_existence=True) - return self._jobStore.import_file(src_uri, shared_file_name=shared_file_name, symlink=symlink) + full_uri = self.normalize_uri(src_uri, check_existence=check_existence) + try: + imported = self._jobStore.import_file(full_uri, shared_file_name=shared_file_name, symlink=symlink) + except FileNotFoundError: + # TODO: I thought we refactored the different job store import + # methods to not raise and instead return None, but that looks to + # not currently be the case. + if check_existence: + raise + else: + # So translate the raise-based API if needed. + # TODO: If check_existence is false but a shared file name is + # specified, we have no way to report the lack of file + # existence, since we also return None on success! + return None + if imported is None and shared_file_name is None and check_existence: + # We need to protect the caller from missing files. + # We think a file was missing, and we got None becasuse of it. + # We didn't get None instead because of usign a shared file name. + raise FileNotFoundError(f'Could not find file {src_uri}') + return imported @deprecated(new_function_name='export_file') def exportFile(self, jobStoreFileID: FileID, dstUrl: str) -> None: @@ -1308,7 +1336,7 @@ def normalize_uri(uri: str, check_existence: bool = False) -> str: """ Given a URI, if it has no scheme, prepend "file:". - :param check_existence: If set, raise an error if a URI points to + :param check_existence: If set, raise FileNotFoundError if a URI points to a local file that does not exist. """ if urlparse(uri).scheme == 'file': diff --git a/src/toil/jobStores/abstractJobStore.py b/src/toil/jobStores/abstractJobStore.py index f6f848a5ae..16ed30e67a 100644 --- a/src/toil/jobStores/abstractJobStore.py +++ b/src/toil/jobStores/abstractJobStore.py @@ -43,11 +43,10 @@ from typing_extensions import Literal from urllib.parse import ParseResult, urlparse +from urllib.error import HTTPError from urllib.request import urlopen from uuid import uuid4 -from requests.exceptions import HTTPError - from toil.common import Config, getNodeID, safeUnpickleFromStream from toil.fileStores import FileID from toil.job import (CheckpointJobDescription, @@ -420,6 +419,8 @@ def import_file(self, - 'gs' e.g. gs://bucket/file + Raises FileNotFoundError if the file does not exist. + :param str src_uri: URL that points to a file or object in the storage mechanism of a supported URL scheme e.g. a blob in an AWS s3 bucket. It must be a file, not a directory or prefix. @@ -453,6 +454,8 @@ def _import_file(self, asks the other job store class for a stream and writes that stream as either a regular or a shared file. + Raises FileNotFoundError if the file does not exist. + :param AbstractJobStore otherCls: The concrete subclass of AbstractJobStore that supports reading from the given URL and getting the file size from the URL. @@ -587,6 +590,8 @@ def read_from_url(cls, src_uri: str, writable: IO[bytes]) -> Tuple[int, bool]: """ Read the given URL and write its content into the given writable stream. + Raises FileNotFoundError if the URL doesn't exist. + :return: The size of the file in bytes and whether the executable permission bit is set :rtype: Tuple[int, bool] """ @@ -618,8 +623,12 @@ def _read_from_url(cls, url: ParseResult, writable: IO[bytes]) -> Tuple[int, boo Reads the contents of the object at the specified location and writes it to the given writable stream. + Raises FileNotFoundError if the URL doesn't exist. + Refer to :func:`~AbstractJobStore.importFile` documentation for currently supported URL schemes. + Raises FileNotFoundError if the thing at the URL is not found. + :param ParseResult url: URL that points to a file or object in the storage mechanism of a supported URL scheme e.g. a blob in an AWS s3 bucket. @@ -635,7 +644,7 @@ def _read_from_url(cls, url: ParseResult, writable: IO[bytes]) -> Tuple[int, boo def _write_to_url(cls, readable: Union[IO[bytes], IO[str]], url: ParseResult, executable: bool = False) -> None: """ Reads the contents of the given readable stream and writes it to the object at the - specified location. + specified location. Raises FileNotFoundError if the URL doesn't exist.. Refer to AbstractJobStore.importFile documentation for currently supported URL schemes. @@ -1707,20 +1716,28 @@ def _read_from_url( # We can only retry on errors that happen as responses to the request. # If we start getting file data, and the connection drops, we fail. # So we don't have to worry about writing the start of the file twice. - with closing(urlopen(url.geturl())) as readable: - # Make something to count the bytes we get - # We need to put the actual count in a container so our - # nested function can modify it without creating its own - # local with the same name. - size = [0] - def count(l: int) -> None: - size[0] += l - counter = WriteWatchingStream(writable) - counter.onWrite(count) - - # Do the download - shutil.copyfileobj(readable, counter) - return size[0], False + try: + with closing(urlopen(url.geturl())) as readable: + # Make something to count the bytes we get + # We need to put the actual count in a container so our + # nested function can modify it without creating its own + # local with the same name. + size = [0] + def count(l: int) -> None: + size[0] += l + counter = WriteWatchingStream(writable) + counter.onWrite(count) + + # Do the download + shutil.copyfileobj(readable, counter) + return size[0], False + except HTTPError as e: + if e.code == 404: + # Translate into a FileNotFoundError for detecting + # un-importable files + raise FileNotFoundError(str(url)) from e + else: + raise @classmethod def _get_is_directory(cls, url: ParseResult) -> bool: diff --git a/src/toil/jobStores/googleJobStore.py b/src/toil/jobStores/googleJobStore.py index a141b84891..0c833fcbbe 100644 --- a/src/toil/jobStores/googleJobStore.py +++ b/src/toil/jobStores/googleJobStore.py @@ -27,6 +27,7 @@ InternalServerError, ServiceUnavailable) from google.cloud import exceptions, storage +from google.auth.exceptions import DefaultCredentialsError from toil.jobStores.abstractJobStore import (AbstractJobStore, JobStoreExistsException, @@ -112,25 +113,54 @@ def __init__(self, locator: str) -> None: self.readStatsBaseID = self.statsReadPrefix+self.statsBaseID self.sseKey = None + self.storageClient = self.create_client() - # Determine if we have an override environment variable for our credentials. - # We don't pull out the filename; we just see if a name is there. - self.credentialsFromEnvironment = bool(os.getenv('GOOGLE_APPLICATION_CREDENTIALS', False)) - if self.credentialsFromEnvironment and not os.path.exists(os.getenv('GOOGLE_APPLICATION_CREDENTIALS')): + @classmethod + def create_client(cls) -> storage.Client: + """ + Produce a client for Google Sotrage with the highest level of access we can get. + + Fall back to anonymous access if no project is available, unlike the + Google Storage module's behavior. + + Warn if GOOGLE_APPLICATION_CREDENTIALS is set but not actually present. + """ + + # Determine if we have an override environment variable for our credentials. + # We get the path to check existence, but Google Storage works out what + # to use later by looking at the environment again. + credentials_path: Optional[str] = os.getenv('GOOGLE_APPLICATION_CREDENTIALS', None) + if credentials_path is not None and not os.path.exists(credentials_path): # If the file is missing, complain. # This variable holds a file name and not any sensitive data itself. log.warning("File '%s' from GOOGLE_APPLICATION_CREDENTIALS is unavailable! " "We may not be able to authenticate!", - os.getenv('GOOGLE_APPLICATION_CREDENTIALS')) - - if not self.credentialsFromEnvironment and os.path.exists(self.nodeServiceAccountJson): - # load credentials from a particular file on GCE nodes if an override path is not set - self.storageClient = storage.Client.from_service_account_json(self.nodeServiceAccountJson) - else: - # Either a filename is specified, or our fallback file isn't there. + credentials_path) + + if credentials_path is None and os.path.exists(cls.nodeServiceAccountJson): + try: + # load credentials from a particular file on GCE nodes if an override path is not set + return storage.Client.from_service_account_json(cls.nodeServiceAccountJson) + except OSError: + # Probably we don't have permission to use the file. + log.warning("File '%s' exists but didn't work to authenticate!", + cls.nodeServiceAccountJson) + pass + + # Either a filename is specified, or our fallback file isn't there. + try: # See if Google can work out how to authenticate. - self.storageClient = storage.Client() + return storage.Client() + except (DefaultCredentialsError, EnvironmentError): + # Depending on which Google codepath or module version (???) + # realizes we have no credentials, we can get an EnvironemntError, + # or the new DefaultCredentialsError we are supposedly specced to + # get. + + # Google can't find credentials, fall back to being anonymous. + # This is likely to happen all the time so don't warn. + return storage.Client.create_anonymous_client() @google_retry @@ -244,10 +274,11 @@ def get_env(self): env = {} - if self.credentialsFromEnvironment: + credentials_path: Optional[str] = os.getenv('GOOGLE_APPLICATION_CREDENTIALS', None) + if credentials_path is not None: # Send along the environment variable that points to the credentials file. # It must be available in the same place on all nodes. - env['GOOGLE_APPLICATION_CREDENTIALS'] = os.getenv('GOOGLE_APPLICATION_CREDENTIALS') + env['GOOGLE_APPLICATION_CREDENTIALS'] = credentials_path return env @@ -351,8 +382,8 @@ def _get_blob_from_url(cls, url, exists=False): if fileName.startswith('/'): fileName = fileName[1:] - storageClient = storage.Client() - bucket = storageClient.get_bucket(bucketName) + storageClient = cls.create_client() + bucket = storageClient.bucket(bucket_name=bucketName) blob = bucket.blob(compat_bytes(fileName)) if exists: diff --git a/src/toil/lib/aws/iam.py b/src/toil/lib/aws/iam.py index a6cadd7420..bece2e0c19 100644 --- a/src/toil/lib/aws/iam.py +++ b/src/toil/lib/aws/iam.py @@ -7,7 +7,7 @@ import boto3 from mypy_boto3_iam import IAMClient -from mypy_boto3_iam.type_defs import AttachedPolicyOutputTypeDef +from mypy_boto3_iam.type_defs import AttachedPolicyTypeDef from mypy_boto3_sts import STSClient from toil.lib.aws import zone_to_region @@ -145,7 +145,7 @@ def get_actions_from_policy_document(policy_doc: Dict[str, Any]) -> AllowedActio allowed_actions[resource][key].append(statement[key]) return allowed_actions -def allowed_actions_attached(iam: IAMClient, attached_policies: List[AttachedPolicyOutputTypeDef]) -> AllowedActionCollection: +def allowed_actions_attached(iam: IAMClient, attached_policies: List[AttachedPolicyTypeDef]) -> AllowedActionCollection: """ Go through all attached policy documents and create an AllowedActionCollection representing granted permissions. diff --git a/src/toil/test/__init__.py b/src/toil/test/__init__.py index 6f51bb724c..582349268d 100644 --- a/src/toil/test/__init__.py +++ b/src/toil/test/__init__.py @@ -409,18 +409,25 @@ def needs_aws_batch(test_item: MT) -> MT: ) return test_item - -def needs_google(test_item: MT) -> MT: +def needs_google_storage(test_item: MT) -> MT: """ Use as a decorator before test classes or methods to run only if Google - Cloud is usable. + Cloud is installed and we ought to be able to access public Google Storage + URIs. """ - test_item = _mark_test('google', test_item) + test_item = _mark_test('google-storage', test_item) try: from google.cloud import storage # noqa except ImportError: return unittest.skip("Install Toil with the 'google' extra to include this test.")(test_item) + return test_item + +def needs_google_project(test_item: MT) -> MT: + """ + Use as a decorator before test classes or methods to run only if we have a Google Cloud project set. + """ + test_item = _mark_test('google-project', test_item) test_item = needs_env_var('TOIL_GOOGLE_PROJECTID', "a Google project ID")(test_item) return test_item @@ -582,6 +589,21 @@ def needs_singularity(test_item: MT) -> MT: return test_item else: return unittest.skip("Install singularity to include this test.")(test_item) + +def needs_singularity_or_docker(test_item: MT) -> MT: + """ + Use as a decorator before test classes or methods to only run them if + docker is installed and docker-based tests are enabled, or if Singularity + is installed. + """ + + # TODO: Is there a good way to OR decorators? + if which('singularity'): + # Singularity is here, say it's a Singularity test + return needs_singularity(test_item) + else: + # Otherwise say it's a Docker test. + return needs_docker(test_item) def needs_local_cuda(test_item: MT) -> MT: """ diff --git a/src/toil/test/jobStores/jobStoreTest.py b/src/toil/test/jobStores/jobStoreTest.py index 1f6a0a9eaa..b0115d1e62 100644 --- a/src/toil/test/jobStores/jobStoreTest.py +++ b/src/toil/test/jobStores/jobStoreTest.py @@ -48,18 +48,17 @@ make_tests, needs_aws_s3, needs_encryption, - needs_google, + needs_google_project, + needs_google_storage, slow) # noinspection PyPackageRequirements # (installed by `make prepare`) -# Need google_retry decorator even if google is not available, so make one up. -# Unconventional use of decorator to determine if google is enabled by seeing if -# it returns the parameter passed in. -if needs_google(needs_google) is needs_google: +try: from toil.jobStores.googleJobStore import google_retry -else: +except ImportError: + # Need google_retry decorator even if google is not available, so make one up. def google_retry(x): return x @@ -1239,7 +1238,8 @@ def test_file_link_imports(self): os.remove(srcUrl[7:]) -@needs_google +@needs_google_project +@needs_google_storage @pytest.mark.xfail class GoogleJobStoreTest(AbstractJobStoreTest.Test): projectID = os.getenv('TOIL_GOOGLE_PROJECTID') @@ -1528,4 +1528,4 @@ def do_GET(self): self.wfile.write(self.fileContents) -AbstractJobStoreTest.Test.makeImportExportTests() \ No newline at end of file +AbstractJobStoreTest.Test.makeImportExportTests() diff --git a/src/toil/test/provisioners/gceProvisionerTest.py b/src/toil/test/provisioners/gceProvisionerTest.py index b09d94ae2e..dd465db826 100644 --- a/src/toil/test/provisioners/gceProvisionerTest.py +++ b/src/toil/test/provisioners/gceProvisionerTest.py @@ -22,7 +22,8 @@ from toil.test import (ToilTest, integrative, needs_fetchable_appliance, - needs_google, + needs_google_project, + needs_google_storage, slow, timeLimit) from toil.version import exactPython @@ -30,7 +31,8 @@ log = logging.getLogger(__name__) -@needs_google +@needs_google_project +@needs_google_storage @integrative @needs_fetchable_appliance @slow @@ -68,7 +70,7 @@ def cleanJobStoreUtil(self): def __init__(self, methodName): super().__init__(methodName=methodName) - # TODO: add TOIL_GOOGLE_KEYNAME to needs_google or ssh with SA account + # TODO: add TOIL_GOOGLE_KEYNAME to needs_google_project or ssh with SA account self.keyName = os.getenv('TOIL_GOOGLE_KEYNAME') # TODO: remove this when switching to google jobstore self.botoDir = os.getenv('TOIL_BOTO_DIR') @@ -210,14 +212,16 @@ def launchCluster(self): # TODO: aren't these checks inherited? @integrative - @needs_google + @needs_google_project + @needs_google_storage def testAutoScale(self): self.instanceTypes = ["n1-standard-2"] self.numWorkers = ['2'] self._test() @integrative - @needs_google + @needs_google_project + @needs_google_storage def testSpotAutoScale(self): self.instanceTypes = ["n1-standard-2:%f" % self.spotBid] # Some spot workers have a stopped state after being started, strangely. @@ -293,7 +297,8 @@ def _runScript(self, toilOptions): self.sshUtil(runCommand) @integrative - @needs_google + @needs_google_project + @needs_google_storage def testAutoScale(self): self.instanceTypes = ["n1-standard-2", "n1-standard-4"] self.numWorkers = ['2','1'] diff --git a/src/toil/test/sort/sortTest.py b/src/toil/test/sort/sortTest.py index 17c884dbe5..04455c592a 100755 --- a/src/toil/test/sort/sortTest.py +++ b/src/toil/test/sort/sortTest.py @@ -30,7 +30,8 @@ from toil.lib.bioio import root_logger from toil.test import (ToilTest, needs_aws_ec2, - needs_google, + needs_google_project, + needs_google_storage, needs_gridengine, needs_mesos, needs_parasol, @@ -197,11 +198,13 @@ def testFileMesos(self): finally: self._stopMesos() - @needs_google + @needs_google_project + @needs_google_storage def testGoogleSingle(self): self._toilSort(jobStoreLocator=self._googleJobStore(), batchSystem="single_machine") - @needs_google + @needs_google_project + @needs_google_storage @needs_mesos def testGoogleMesos(self): self._startMesos() diff --git a/src/toil/test/src/fileStoreTest.py b/src/toil/test/src/fileStoreTest.py index a4eb8a16b4..2babe4d5e1 100644 --- a/src/toil/test/src/fileStoreTest.py +++ b/src/toil/test/src/fileStoreTest.py @@ -36,7 +36,7 @@ from toil.jobStores.abstractJobStore import NoSuchFileException from toil.exceptions import FailedJobsException from toil.realtimeLogger import RealtimeLogger -from toil.test import ToilTest, needs_aws_ec2, needs_google, slow +from toil.test import ToilTest, needs_aws_ec2, needs_google_project, needs_google_storage, slow # Some tests take too long on the AWS jobstore and are unquitable for CI. They can be # be run during manual tests by setting this to False. @@ -1350,13 +1350,15 @@ class CachingFileStoreTestWithAwsJobStore(hidden.AbstractCachingFileStoreTest): jobStoreType = 'aws' -@needs_google +@needs_google_project +@needs_google_storage class NonCachingFileStoreTestWithGoogleJobStore(hidden.AbstractNonCachingFileStoreTest): jobStoreType = 'google' @slow -@needs_google +@needs_google_project +@needs_google_storage @pytest.mark.timeout(1000) class CachingFileStoreTestWithGoogleJobStore(hidden.AbstractCachingFileStoreTest): jobStoreType = 'google' diff --git a/src/toil/test/wdl/md5sum/md5sum-gs.json b/src/toil/test/wdl/md5sum/md5sum-gs.json new file mode 100644 index 0000000000..db1af78437 --- /dev/null +++ b/src/toil/test/wdl/md5sum/md5sum-gs.json @@ -0,0 +1 @@ +{"ga4ghMd5.inputFile": "gs://broad-public-datasets/NA12878/NA12878.cram.crai"} diff --git a/src/toil/test/wdl/wdltoil_test.py b/src/toil/test/wdl/wdltoil_test.py index 050c6ef207..da7b6a3863 100644 --- a/src/toil/test/wdl/wdltoil_test.py +++ b/src/toil/test/wdl/wdltoil_test.py @@ -9,7 +9,7 @@ import pytest -from toil.test import ToilTest, needs_docker, needs_docker_cuda, needs_java, needs_singularity, slow +from toil.test import ToilTest, needs_docker, needs_docker_cuda, needs_google_storage, needs_java, needs_singularity_or_docker, slow from toil.version import exactPython # Don't import the test case directly or pytest will test it again. import toil.test.wdl.toilwdlTest @@ -26,9 +26,9 @@ def setUpClass(cls) -> None: # We inherit a testMD5sum but it is going to need Singularity and not # Docker now. And also needs to have a WDL 1.0+ WDL file. So we replace it. - @needs_singularity + @needs_singularity_or_docker def testMD5sum(self): - """Test if toilwdl produces the same outputs as known good outputs for WDL's + """Test if Toil produces the same outputs as known good outputs for WDL's GATK tutorial #1.""" wdl = os.path.abspath('src/toil/test/wdl/md5sum/md5sum.1.0.wdl') json_file = os.path.abspath('src/toil/test/wdl/md5sum/md5sum.json') @@ -53,7 +53,7 @@ def test_empty_file_path(self): assert retval != 0 assert b'Could not find' in stderr - @needs_singularity + @needs_singularity_or_docker def test_miniwdl_self_test(self): """Test if the MiniWDL self test runs and produces the expected output.""" wdl_file = os.path.abspath('src/toil/test/wdl/miniwdl_self_test/self_test.wdl') @@ -85,7 +85,6 @@ def test_miniwdl_self_test(self): @slow @needs_docker_cuda - @needs_singularity def test_giraffe_deepvariant(self): """Test if Giraffe and CPU DeepVariant run. This could take 25 minutes.""" # TODO: enable test if nvidia-container-runtime and Singularity are installed but Docker isn't. @@ -128,7 +127,7 @@ def test_giraffe_deepvariant(self): assert os.path.exists(outputs['GiraffeDeepVariant.output_vcf']) @slow - @needs_singularity + @needs_singularity_or_docker def test_giraffe(self): """Test if Giraffe runs. This could take 12 minutes. Also we scale it down.""" # TODO: enable test if nvidia-container-runtime and Singularity are installed but Docker isn't. @@ -155,5 +154,32 @@ def test_giraffe(self): assert isinstance(outputs['Giraffe.output_bam'], str) assert os.path.exists(outputs['Giraffe.output_bam']) + @needs_singularity_or_docker + @needs_google_storage + def test_gs_uri(self): + """Test if Toil can access Google Storage URIs.""" + wdl = os.path.abspath('src/toil/test/wdl/md5sum/md5sum.1.0.wdl') + json_file = os.path.abspath('src/toil/test/wdl/md5sum/md5sum-gs.json') + + result_json = subprocess.check_output(self.base_command + [wdl, json_file, '-o', self.output_dir, '--logDebug']) + result = json.loads(result_json) + + assert 'ga4ghMd5.value' in result + assert isinstance(result['ga4ghMd5.value'], str) + assert os.path.exists(result['ga4ghMd5.value']) + assert os.path.basename(result['ga4ghMd5.value']) == 'md5sum.txt' + + def test_empty_file_path(self): + """Test if empty File type inputs are protected against""" + wdl = os.path.abspath('src/toil/test/wdl/md5sum/md5sum.1.0.wdl') + json_file = os.path.abspath('src/toil/test/wdl/md5sum/empty_file.json') + + p = subprocess.Popen(self.base_command + [wdl, json_file, '-o', self.output_dir, '--logDebug'], stdout=subprocess.PIPE, stderr=subprocess.PIPE) + stdout, stderr = p.communicate() + retval = p.wait() + + assert retval != 0 + assert b'Could not find' in stderr + if __name__ == "__main__": unittest.main() # run all tests diff --git a/src/toil/wdl/wdltoil.py b/src/toil/wdl/wdltoil.py index 651618598a..78f6070a11 100755 --- a/src/toil/wdl/wdltoil.py +++ b/src/toil/wdl/wdltoil.py @@ -708,7 +708,9 @@ def import_file_from_uri(uri: str) -> str: # Try each place it could be according to WDL finding logic. tried.append(candidate_uri) try: - imported = toil.import_file(candidate_uri) + # Try to import the file. Don't raise if we can't find it, just + # return None! + imported = toil.import_file(candidate_uri, check_existence=False) except UnimplementedURLException as e: # We can't find anything that can even support this URL scheme. # Report to the user, they are probably missing an extra. diff --git a/version_template.py b/version_template.py index 313e805625..29e7463a78 100644 --- a/version_template.py +++ b/version_template.py @@ -39,6 +39,28 @@ def version(): """ return '-'.join(filter(None, [distVersion(), currentCommit(), ('dirty' if dirty() else None)])) +def cacheTag(): + """ + A Docker tag that we should use to cache Docker image build layers for this commit. + """ + + import os + return ''.join([ + "cache-", + # Pick up branch or tag from Gitlagb CI, or just use "local" for everyone. + ((os.getenv('CI_COMMIT_BRANCH', '') + os.getenv('CI_COMMIT_TAG', '')) or 'local').replace('/', '-'), + _pythonVersionSuffix() + ]) + +def mainCacheTag(): + """ + A Docker tag where the Toil mainline builds cache their layers. + """ + + return ''.join([ + "cache-master", + _pythonVersionSuffix() + ]) def distVersion(): """The distribution version identifying a published release on PyPI."""