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

[Internal] Updating runtime parameters #494

Merged
merged 15 commits into from
Jun 7, 2024
Merged

[Internal] Updating runtime parameters #494

merged 15 commits into from
Jun 7, 2024

Conversation

sage-wright
Copy link
Member

@sage-wright sage-wright commented May 29, 2024

This one is a doozy.

This PR:

🗑️ This dev branch should be deleted after merging to main.

🧠 Aim, Context and Functionality

Many tasks require far more runtime parameters than are actually used. This PR adjusts various runtime parameters accordingly.

🛠️ Impacted Workflows/Tasks & Changes Being Made

This will affect the behavior of the workflow(s) even if users don’t change any workflow inputs relative to the last version : No

Running this workflow on different occasions could result in different results, e.g. due to use of a live database, "latest" docker image, or stochastic data processing : No

  • No change in workflow function, just optimization of resources

📋 Workflow/Task Step Changes

🔄 Data Processing

Compute resources changed:

  • many, see also code changes

➡️ Inputs

None

⬅️ Outputs

None

🧪 Testing

Terra Testing

Suggested Scenarios for Reviewer to Test

Ensure costs and runtime attributes went down as expected.

Theiagen Version Release Testing (optional)

None

🔬 Final Developer Checklist

  • The workflow/task has been tested locally and results, including file contents, are as anticipated
  • The workflow/task has been tested on Terra and results, including file contents, are as anticipated
  • The CI/CD has been adjusted and tests are passing (to be completed by Theiagen developer)
  • Code changes follow the style guide

🎯 Reviewer Checklist

  • All impacted workflows/tasks have been tested on Terra with a different dataset than used for development
  • All reviewer-suggested scenarios have been tested and any additional
  • All changed results have been confirmed to be accurate
  • All workflows/tasks impacted by change/s have been tested using a standard validation dataset to ensure no unintended change of functionality
  • All code adheres to the style guide
  • MD5 sums have been updated
  • The PR author has addressed all comments

🗂️ Associated Documentation (to be completed by Theiagen developer)

  • Relevant documentation on the Public Health Resources "PHB Main" has been updated
  • Workflow diagrams have been updated to reflect changes

@sage-wright sage-wright linked an issue May 29, 2024 that may be closed by this pull request
@sage-wright sage-wright marked this pull request as ready for review May 31, 2024 14:41
@AndrewLangvt
Copy link
Contributor

TheiaProk_Illumina_PE_PHB - https://app.terra.bio/#workspaces/theiagen-training-workspaces/Theiagen_Lang_Sandbox/job_history/4fa24d88-2417-4fa3-a3e1-3f4268ca1198
shovil - confirmed CPUs are passed to shovil command
busco - confirmed preemtible
gambit - confirmed disk size 20, confirmed mem 2, CPU 1

realized I had to pass in flag to run midas- https://app.terra.bio/#workspaces/theiagen-training-workspaces/Theiagen_Lang_Sandbox/job_history/1fa8a187-17f8-4638-9ba3-965adb8e1c73
midas - confirmed preemtible, mem = 4

@jrotieno from my perspective, once you review your set of these workflows, you can mark it approved for merge

@jrotieno
Copy link
Contributor

jrotieno commented Jun 7, 2024

For the additional TheiaProk tasks assigned to me, I just used Andrew Lang's runs:
check-reads:
2GB RAM, 100GB Disk, 1 CPU and preemptible TRUE
cg-pipeline:
2GB RAM, 50GB Disk, 1 CPU and preemptible TRUE
version_capture
container used is the alpine-plus-bash. Not seen the version though (3.20.0), should I have in the compute details?
1GB RAM, 10GB Disk, 1 CPU and preemptible TRUE

kSNP3 CPUs:
https://job-manager.dsde-prod.broadinstitute.org/jobs/b7e86f67-8764-49d4-8456-b0d3d7f7944a
4GB RAM, 100GB Disk, 2 CPU and preemptible FALSE

@jrotieno
Copy link
Contributor

jrotieno commented Jun 7, 2024

@AndrewLangvt maybe you just need to mark the ones you went through as done on the board?

@AndrewLangvt
Copy link
Contributor

@jrotieno good call; done!

@jrotieno jrotieno self-requested a review June 7, 2024 17:50
Copy link
Contributor

@jrotieno jrotieno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndrewLangvt and I have reviewed the changes and approve. Well done @sage-wright

@jrotieno jrotieno merged commit 4499c85 into main Jun 7, 2024
26 checks passed
@sage-wright sage-wright deleted the smw-resources-dev branch June 14, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants