Integrate dsub #195

jhiemstrawisc · 2024-11-18T21:35:58Z

Opening this PR in draft mode for now while @nisthapanda work through collaborating on documentation updates and other testing.

Merge Reed Comp Bio SPRAS master branch

…mporary fix to use dsub as framework with oi1, change to gcloud storage cp for larger file transfers

…of larger files

…dsub file paths are handled in container command

agitter · 2024-11-22T23:10:04Z

Some additional context: we started using dsub for All of Us

Within the Researcher Workbench, the Google Cloud Life Sciences API is the executor of dsub tasks.

However

Cloud Life Sciences is deprecated and will no longer be available on Google Cloud after July 8, 2025.

It's not yet clear what All of Us will do to support migration. I would still like to proceed with merging in this code though, but we may be okay keeping documentation light if our use case is already deprecated.

See also #130.

As Tony pointed out in the PR review, Google Cloud Life Sciences is being deprecated, so we're not sure how much longer this will be around. Instead of going through and giving the `dsub` framework verbose documentation, we explicitly state it's usage is experimental and warn the user that they should already know what they're doing if they plan to use it.

This prints a warning for the user if they select the `dsub` framework. Since we're unsure whether we'll need to support dsub in the future (as our primary motivation for using it is being deprecated), this warning should prevent potential users from getting their hopes up that we have long-term plans for it.

jhiemstrawisc · 2024-12-10T15:14:16Z

@agitter, I believe @nisthapanda tested this integration with GCLS and verified that the merge didn't break anything for her. Since we're not sure what the long-term plan is for dsub integration, I left minimal documentation in config/config.py that mentions it's experimental, along with a runtime warning to that effect.

Since this isn't breaking tests in either direction, I think it's ready for a proper review.

agitter

The changes look pretty good overall. I mostly made small formatting suggestions.

agitter · 2025-01-17T23:11:20Z

spras/config.py

@@ -114,9 +114,11 @@ def process_config(self, raw_config):
        # However, if we get a bad value, we raise an exception.
        if "container_framework" in raw_config:
            container_framework = raw_config["container_framework"].lower()
-            if container_framework not in ("docker", "singularity"):
+            if container_framework not in ("docker", "singularity", "dsub"):
                msg = "SPRAS was configured to run with an unknown container framework: '" + raw_config["container_framework"] + "'. Accepted values are 'docker' or 'singularity'."


Need to update the options in this line too

agitter · 2025-01-17T23:16:21Z

spras/containers.py

@@ -41,6 +42,79 @@ def convert_docker_path(src_path: PurePath, dest_path: PurePath, file_path: Unio
    rel_path = file_path.relative_to(src_path)
    return PurePosixPath(dest_path, rel_path)

+def download_gcs(gcs_path: str, local_path: str, is_dir: bool):


My IDE shows a warning requesting 2 blank lines before these new functions.

agitter · 2025-01-17T23:20:33Z

spras/containers.py

+    # run command
+    subprocess.run(cmd, shell=True)
+
+    if is_dir and Path(Path(local_path)/'gcs_temp.txt').exists():


@nisthapanda why do we check is_dir here?

I don't think we need the is_dir here.

agitter · 2025-01-17T23:21:07Z

spras/containers.py

+    subprocess.run(cmd, shell=True)
+
+    if is_dir and Path(Path(local_path)/'gcs_temp.txt').exists():
+        os.remove(Path(Path(local_path)/'gcs_temp.txt'))


Suggested change

os.remove(Path(Path(local_path)/'gcs_temp.txt'))

Path(Path(local_path)/'gcs_temp.txt').unlink()

Does this work? It's a bit simpler to me to stick with pathlib operations.

This works.

agitter · 2025-01-17T23:22:33Z

spras/containers.py

+def upload_gcs(local_path: str, gcs_path: str, is_dir: bool):
+    # check if path exists in cloud storage
+    exists = len(subprocess.run(f'gcloud storage ls {gcs_path}', shell=True, capture_output=True, text=True).stdout)
+    # if path exists rsyc


Suggested change

# if path exists rsyc

# if path exists rsync

agitter · 2025-01-17T23:27:54Z

spras/containers.py

+    current owner and group IDs.
+    Does not modify the owner or group for existing files modified by the container.
+    @param container: name of the container in the Google Cloud Container Registry
+    @param command: command to run in the container


Suggested change

@param command: command to run in the container

@param command: command to run

agitter · 2025-01-17T23:28:56Z

spras/containers.py

@@ -258,3 +334,74 @@ def prepare_volume(filename: Union[str, PurePath], volume_base: Union[str, PureP
        src = parent

    return (src, dest), container_filename
+
+def run_container_dsub(container: str, command: List[str], volumes: List[Tuple[PurePath, PurePath]], working_dir: str, environment: str = 'SPRAS=True'):


Suggested change

def run_container_dsub(container: str, command: List[str], volumes: List[Tuple[PurePath, PurePath]], working_dir: str, environment: str = 'SPRAS=True'):

def run_container_dsub(container: str, command: List[str], volumes: List[Tuple[PurePath, PurePath]], working_dir: str, environment: str = 'SPRAS=True') -> str:

agitter · 2025-01-17T23:29:32Z

spras/containers.py

+
+    workspace_bucket = os.getenv('WORKSPACE_BUCKET')
+    # Add path in the workspace bucket and label for dsub command for each volume
+    dsub_volumes = [(src, dst, workspace_bucket +  str(dst), "INPUT_" + str(i),) for i, (src, dst) in enumerate(volumes)]


Suggested change

dsub_volumes = [(src, dst, workspace_bucket + str(dst), "INPUT_" + str(i),) for i, (src, dst) in enumerate(volumes)]

dsub_volumes = [(src, dst, workspace_bucket + str(dst), "INPUT_" + str(i),) for i, (src, dst) in enumerate(volumes)]

agitter · 2025-01-17T23:31:11Z

spras/containers.py

+    flags['image'] = container
+    flags['env'] = environment
+    flags['input-recursive'] = [vol[3]+'='+vol[2] for vol in dsub_volumes]
+    flags['output-recursive'] = "OUTPUT=" + workspace_bucket +  working_dir


Suggested change

flags['output-recursive'] = "OUTPUT=" + workspace_bucket + working_dir

flags['output-recursive'] = "OUTPUT=" + workspace_bucket + working_dir

agitter · 2025-01-17T23:31:38Z

spras/containers.py

+        download_gcs(local_path=str(src), gcs_path=gcs_path, is_dir=True)
+
+    # return location of dsub logs in WORKSPACE_BUCKET
+    return  'dsub logs: {logs}'.format(logs = flags['logging'])


Suggested change

return 'dsub logs: {logs}'.format(logs = flags['logging'])

return 'dsub logs: {logs}'.format(logs=flags['logging'])

nisthapanda and others added 30 commits September 26, 2023 11:09

Add dsub to conda environment

2062294

Add functions for running SPRAS with dsub in All of Us

863bdcf

Merge remote-tracking branch 'upstream/master' into dsub

152d762

Add functions for running with dsub in All of Us

92c2a97

Resolved volume mapping issues

61976b7

Change location of dsub logs in Google Cloud Storage

1725d1b

Merge branch 'master' into dsub

2aac5f2

Merge pull request #1 from nisthapanda/dsub

bb1af98

Merge Reed Comp Bio SPRAS master branch

Merge remote-tracking branch 'spras/direction' into dsub

f0134a3

Merge SPRAS updates

d702721

Move run_container_dsub and helper functions to contianers.py, add te…

616cd67

…mporary fix to use dsub as framework with oi1, change to gcloud storage cp for larger file transfers

Update dsub version in environment.yml

11d0450

Update dsub version in environment.yml

2769ab0

Update sphinx-rtd-theme version in environment.yml

6b2a3c3

Merge branch 'Reed-CompBio:master' into dsub

4b8a66a

Change to use gcloud storage rsync to copy files for faster transfer …

28682a8

…of larger files

Change the the container tag to match container in GCR

2f8723a

Merge branch 'Reed-CompBio:master' into dsub

8634119

Merge branch 'Reed-CompBio:master' into dsub

b712711

Remove temporary solution for running OI1 with dsub

cea8505

Add dsub as a possible framework

6d6f4bb

Create workaround for uploading empty directories to GCS, Change how …

816ec07

…dsub file paths are handled in container command

Add dsub volume mount prefix to paths in meo-properties.txt file

e9be16b

Remove unused package and extra spaces

53eaf35

Remove print statement, remove trailing spaces

ce951cd

Remove trailing spaces, code formatting

c225dcf

Merge branch 'Reed-CompBio:master' into dsub

d0172fb

Merge SPRAS master

88105ee

Add dependencies from dsub to conda environment

5452b6f

Merge branch 'Reed-CompBio:master' into dsub

0bf57a3

jhiemstrawisc added 2 commits December 10, 2024 08:59

jhiemstrawisc marked this pull request as ready for review December 10, 2024 15:08

agitter reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate dsub #195

Integrate dsub #195

jhiemstrawisc commented Nov 18, 2024

agitter commented Nov 22, 2024

jhiemstrawisc commented Dec 10, 2024

agitter left a comment

agitter Jan 17, 2025

agitter Jan 17, 2025

agitter Jan 17, 2025

nisthapanda Jan 18, 2025

agitter Jan 17, 2025

nisthapanda Jan 18, 2025

agitter Jan 17, 2025

agitter Jan 17, 2025

agitter Jan 17, 2025

agitter Jan 17, 2025

agitter Jan 17, 2025

agitter Jan 17, 2025

	os.remove(Path(Path(local_path)/'gcs_temp.txt'))
	Path(Path(local_path)/'gcs_temp.txt').unlink()

	@param command: command to run in the container
	@param command: command to run

	def run_container_dsub(container: str, command: List[str], volumes: List[Tuple[PurePath, PurePath]], working_dir: str, environment: str = 'SPRAS=True'):
	def run_container_dsub(container: str, command: List[str], volumes: List[Tuple[PurePath, PurePath]], working_dir: str, environment: str = 'SPRAS=True') -> str:

	dsub_volumes = [(src, dst, workspace_bucket + str(dst), "INPUT_" + str(i),) for i, (src, dst) in enumerate(volumes)]
	dsub_volumes = [(src, dst, workspace_bucket + str(dst), "INPUT_" + str(i),) for i, (src, dst) in enumerate(volumes)]

	flags['output-recursive'] = "OUTPUT=" + workspace_bucket + working_dir
	flags['output-recursive'] = "OUTPUT=" + workspace_bucket + working_dir

	return 'dsub logs: {logs}'.format(logs = flags['logging'])
	return 'dsub logs: {logs}'.format(logs=flags['logging'])

Integrate dsub #195

Are you sure you want to change the base?

Integrate dsub #195

Conversation

jhiemstrawisc commented Nov 18, 2024

agitter commented Nov 22, 2024

jhiemstrawisc commented Dec 10, 2024

agitter 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment