Feat: Adding the option to provide cpu count and memory size for the … #413

josuebc · 2022-04-20T18:31:19Z

Adding the option to provide cpu count and memory size for the engine via the yaml project file.

Issue: #204

Description of Changes
I added the ability to change the cpu count and memory limit of the engine within a specific context.

These was accomplish by adding two new variables that get included on the call to the backend, they get included with all existing variables.

In the backend these new variables will only be used by the engines logic when setting up the workflow via "agc context deploy --context "

engines in context for these changes are: Cromwell, Toil, miniwdl, nextflow and snakemake .

Description of how you validated changes
Unit Test for the changes were verified all pass.
Exiting unit test affected by the changes were update and validated.

Lastly I build the CLI and ran multiple variation for all engines how the end user would update the project file and verified all got reflected as expected in the workflows

agc account activate
agc context deploy --context spotContext
agc workflow run hello --context spotContext

Checklist

If this change would make any existing documentation invalid, I have included those updates within this PR
I have added/updated unit tests that prove my fix is effective or that my feature works
I have linted my code before raising the PR
Title of this Pull Request follows Conventional Commits standard: https://www.conventionalcommits.org/en/v1.0.0/

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

…engine via the yaml project file.

examples/demo-nextflow-project/agc-project.yaml

IllyaYalovyy · 2022-04-27T17:05:45Z

packages/cdk/lib/env/context-app-parameters.ts

+   */
+  public readonly vCpus?: number;
+  /**
+   * The maximum memory limit that an environment can reach for the nextflow engine.


Is this NextFlow specific? How it will affect other supported engines? Have we tested this change with other engines?

No, changes got updated to support all engines
I tested for expected behavior was performed on all engines.

Should you update comments than?

…engine via the yaml project file.

markjschreiber

The JSON schema and binding would allow you to provide these parameters for any engine which creates the impression you can do it for all. The docs also don't say this is only for Nextflow.

Is there a reason that the approach cannot be generalized to all engines? Or at least those engines that run as Batch Jobs (miniwdl, nextflow and snakemake).

…engine via the yaml project file.

markjschreiber

Would it be possible to extend this approach to include Cromwell and Toil? Although they run as ECS tasks rather than Batch jobs they have similarly configurable resource requirements. You would need to make sure that the minimum values are what they are now.

The ECS services are created by packages/cdk/lib/constructs/secure-service.ts and the cpu and memory values are in the SecureServiceProps

examples/demo-wdl-project/agc-project.yaml

packages/cdk/lib/constructs/engines/engine-job-definition.ts

…all engines via the yaml project file.

site/content/en/docs/Concepts/contexts.md

markjschreiber

One minor suggestion in the docs but otherwise great.

Co-authored-by: Mark Schreiber <[email protected]>

IllyaYalovyy · 2022-06-03T20:33:22Z

packages/cdk/lib/constructs/engines/snakemake/snakemake-engine.ts

      platformCapabilities: [PlatformCapabilities.FARGATE],
      container: {
-        memoryLimitMiB: this.engineMemoryMiB,
+        vcpus: props.vcpus || 1,


can you create a constant with a meaningful name for this magic number?

const defaultVCPUs = 1;

IllyaYalovyy · 2022-06-03T20:33:53Z

packages/cdk/lib/constructs/engines/snakemake/snakemake-engine.ts

      container: {
-        memoryLimitMiB: this.engineMemoryMiB,
+        vcpus: props.vcpus || 1,
+        memoryLimitMiB: props.engineMemoryMiB || 4096,


can you create a constant with a meaningful name for this magic number?

const defaultMemoryMiB = 4096;

IllyaYalovyy · 2022-06-03T20:34:43Z

packages/cdk/lib/env/context-app-parameters.ts

+   */
+  public readonly vCpus?: number;
+  /**
+   * The maximum memory limit that an environment can reach for the nextflow engine.


Should you update comments than?

IllyaYalovyy · 2022-06-03T20:36:40Z

packages/cdk/lib/stacks/engines/cromwell-engine-construct.ts


    // TODO: Move log group creation into service construct and make it a property
-    this.engine = this.getEngineServiceDefinition(props.vpc, props.subnets, engineContainer, this.engineLogGroup);
+    this.engine = this.getEngineServiceDefinition(props.vpc, props.subnets, engineContainer, this.engineLogGroup, params);


I think you just cut in between the line of code and the comment that corresponds to that line of code. Now this TODO comment sounds out of context. Could you please rearrange code, so the comment stays close to the related line?

IllyaYalovyy · 2022-06-03T20:38:34Z

packages/cdk/lib/stacks/engines/cromwell-engine-construct.ts

      taskRole: this.engineRole,
-      cpu: serviceContainer.cpu,
-      memoryLimitMiB: serviceContainer.memoryLimitMiB,
+      cpu: (params.vCpus ? params.vCpus * 1024 : undefined) || serviceContainer.cpu,


What is the meaning of this 1024 magic number? can you use a constant with a meaningful name instead? I think it will improve readability and will serve self-documenting purpose.

IllyaYalovyy · 2022-06-03T20:39:27Z

packages/cdk/lib/stacks/engines/cromwell-engine-construct.ts

    const container = definition.addContainer(serviceContainer.serviceName, {
-      cpu: serviceContainer.cpu,
-      memoryLimitMiB: serviceContainer.memoryLimitMiB,
+      cpu: (params.vCpus ? params.vCpus * 1024 : undefined) || serviceContainer.cpu,


ditto. By the way, may be we should consider avoiding this duplicated code block?

IllyaYalovyy · 2022-06-03T20:40:26Z

packages/cdk/lib/stacks/engines/toil-engine-construct.ts

      taskRole: this.engineRole,
-      cpu: serviceContainer.cpu,
-      memoryLimitMiB: serviceContainer.memoryLimitMiB,
+      cpu: (params.vCpus ? params.vCpus * 1024 : undefined) || serviceContainer.cpu,


Here we go again... I think if some number has to be repeated more than once, it should be moved to a constant.

IllyaYalovyy · 2022-06-03T20:40:47Z

packages/cdk/lib/stacks/engines/toil-engine-construct.ts

    definition.addContainer(serviceContainer.serviceName, {
-      cpu: serviceContainer.cpu,
-      memoryLimitMiB: serviceContainer.memoryLimitMiB,
+      cpu: (params.vCpus ? params.vCpus * 1024 : undefined) || serviceContainer.cpu,


and one more time...

IllyaYalovyy · 2022-06-03T20:47:07Z

packages/cli/internal/pkg/cli/spec/context.go

+
+type ResourceRequirement struct {
+	VCpus          int `yaml:"vcpus,omitempty"`
+	MemoryLimitMiB int `yaml:"memoryLimit,omitempty"`


I don't think it will work as you might expect. "omitempty" means not to include values that absent. In your case values always present.

Here is the test:
https://go.dev/play/p/WB4AzY1JhTT

I would suggest a unit test for that.

IllyaYalovyy · 2022-06-03T20:48:19Z

packages/cli/internal/pkg/cli/spec/project_test.go

 								Type:   "nextflow",
 								Engine: "nextflow",
+								ResourceRequirements: ResourceRequirement{
+									VCpus:          2,


Lets add tests with partial configuration. e.g. either VCpus or MemoryLimitMiB is set, but not both.

Feat: Adding the option to provide cpu count and memory size for the …

e6ad46a

…engine via the yaml project file.

josuebc temporarily deployed to slack April 20, 2022 18:31 Inactive

markjschreiber previously approved these changes Apr 26, 2022

View reviewed changes

examples/demo-nextflow-project/agc-project.yaml Outdated Show resolved Hide resolved

IllyaYalovyy reviewed Apr 27, 2022

View reviewed changes

Feat: Adding the option to provide cpu count and memory size for the …

df06b26

…engine via the yaml project file.

josuebc dismissed markjschreiber’s stale review via df06b26 May 4, 2022 21:58

markjschreiber suggested changes May 4, 2022

View reviewed changes

josuebc added 2 commits May 18, 2022 11:07

Feat: Adding the option to provide cpu count and memory size for the …

eeda242

…engine via the yaml project file.

Feat: Adding the option to provide cpu count and memory size for the …

48e327c

…engine via the yaml project file.

josuebc requested review from IllyaYalovyy and markjschreiber May 18, 2022 18:27

Merge branch 'main' into josueba/nextflowConfigEngineSize

e723969

markjschreiber reviewed May 18, 2022

View reviewed changes

examples/demo-wdl-project/agc-project.yaml Outdated Show resolved Hide resolved

packages/cdk/lib/constructs/engines/engine-job-definition.ts Outdated Show resolved Hide resolved

Feat: Adding the option to provide cpu count and memory size for the …

4c9ad65

…all engines via the yaml project file.

josuebc requested a review from markjschreiber May 25, 2022 00:23

markjschreiber reviewed May 25, 2022

View reviewed changes

site/content/en/docs/Concepts/contexts.md Outdated Show resolved Hide resolved

markjschreiber previously approved these changes May 25, 2022

View reviewed changes

josuebc dismissed markjschreiber’s stale review via 7d28f6b May 25, 2022 16:26

Update site/content/en/docs/Concepts/contexts.md

7d28f6b

Co-authored-by: Mark Schreiber <[email protected]>

josuebc requested a review from markjschreiber May 25, 2022 16:42

markjschreiber approved these changes May 25, 2022

View reviewed changes

IllyaYalovyy suggested changes Jun 3, 2022

View reviewed changes

Feat: Adding the option to provide cpu count and memory size for the … #413

Are you sure you want to change the base?

Feat: Adding the option to provide cpu count and memory size for the … #413

Uh oh!

Conversation

josuebc commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josuebc May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markjschreiber left a comment

Choose a reason for hiding this comment

Uh oh!

markjschreiber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markjschreiber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

josuebc commented Apr 20, 2022 •

edited

Loading

josuebc May 4, 2022 •

edited

Loading