Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config and/or modify implementation of execution paths #112

Closed
MattMcL4475 opened this issue Mar 2, 2023 · 1 comment · Fixed by #222
Closed

Add config and/or modify implementation of execution paths #112

MattMcL4475 opened this issue Mar 2, 2023 · 1 comment · Fixed by #222
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@MattMcL4475
Copy link
Collaborator

MattMcL4475 commented Mar 2, 2023

Investigate and propose the changes required to make the batch script have the desired path schema for Terra:

Current:

https://test.blob.core.windows.net/sc-a617a56e-56a5-4a52-b4f0-895afa7ccbe8/cromwell-executions/assemble_refbased/0fb40b27-dffe-4d3f-b6da-02ea0b82aedd/call-align_to_ref/shard-0/execution/__batch/batch_script
/cromwell-executions/assemble_refbased/0fb40b27-dffe-4d3f-b6da-02ea0b82aedd

Desired:

https://test.blob.core.windows.net/sc-a617a56e-56a5-4a52-b4f0-895afa7ccbe8/workspace-services/cbas/wds-a617a56e-56a5-4a52-b4f0-895afa7ccbe8/assemble_refbased/0fb40b27-dffe-4d3f-b6da-02ea0b82aedd/call-align_to_ref/shard-0/execution/__batch/batch_script
/workspace-services/cbas/wds-a617a56e-56a5-4a52-b4f0-895afa7ccbe8
@MattMcL4475 MattMcL4475 added the enhancement New feature or request label Mar 2, 2023
@BMurri BMurri added this to the next milestone Mar 8, 2023
@BMurri
Copy link
Collaborator

BMurri commented Apr 25, 2023

My suggestion:

  1. Drop cromwell-executions, executions, etc. as a TES thing. Those directories are, originally, a CoA thing that was piggybacked to solve the question of where to drop TES-generated scripts to bootstrap/control the executors. They can still be created in the CoA deployer as a convenience for Cromwell, but otherwise such a container is the purview of the deployment team integrating TES into whatever their solution happens to be.
  2. Add a new container. Right now, ga4gs-tes's deployer creates one called executions and uses it assuming it's being called by Cromwell. I suggest a completely different name so as to enforce the concept that it's not simply a generic cromwell-executions. My (awfully named) suggestion: "tes-internal";
  3. Inside of that new container, create a folder that is the TesTask.Id's value. Inside of that folder, stage the files(scripts) that currently go inside of the __batch folder way down at the bottom of each task in cromwell-executions. Those scripts will presume that the executor will root in a container starting in a wd subfolder of this "per-task" folder, will create such a directory in the batch task's working directory, and will set such directory as the root of the executor's filesystem view.
  4. On failure, this same folder is the new destination of the stdout.txt and stderr.txt files, as well as the metrics.txt file.
  5. There is no "public" configuration around this new container. It is NOT a replacement for cromwell-executions.
  6. Trigger Service is altered to look in this new location to generate the per-workflow metrics files that are placed in the Outputs container.

@ngambani ngambani modified the milestones: next, TES Backlog Apr 28, 2023
@ashanhol ashanhol self-assigned this May 1, 2023
@ashanhol ashanhol moved this to In Progress in Current Terra Sprint May 1, 2023
ashanhol added a commit that referenced this issue May 15, 2023
Fixes #112
This PR further aims to untangle CoA from TES by separating cromwell's execution files from TES's batch files. The majority of this work is done in BatchScheduler.cs's ConvertTesTaskToBatchTaskAsync function.

Created a new container in our storage account called tes-internal which is the new location for the upload, download, and batch scripts as well as metrics.txt. New storage path convention is /tes-internal/task.id/[file]
Got rid of Tes's executions container.
Create tes-internal container in CoA deployer: Create tes-internal container during deploy CromwellOnAzure#658
On the batch node, truncated the long paths that were previously something like cromwell-executions/assemble_refbased/0fb40b27-dffe-4d3f-b6da-02ea0b82aedd/call-align_to_ref/shard-0/execution/__batch/batch_script to wd/batch_script.
This required some finagling with the mounting directories.
Updated any tests that were relying on the previous way of doing things.

---------

Co-authored-by: Blair L Murri <BMurri@users.noreply.github.com>
@github-project-automation github-project-automation bot moved this from In Progress to Done in Current Terra Sprint May 15, 2023
ashanhol added a commit to microsoft/CromwellOnAzure that referenced this issue May 15, 2023
Sister PR to microsoft/ga4gh-tes#222
Issue: microsoft/ga4gh-tes#112

Creates the tes-internal container for convenience to TES.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants