-
Notifications
You must be signed in to change notification settings - Fork 78
Feat: Adding the option to provide cpu count and memory size for the … #413
base: main
Are you sure you want to change the base?
Changes from all commits
e6ad46a
df06b26
eeda242
48e327c
e723969
4c9ad65
7d28f6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,14 +12,15 @@ export interface SnakemakeEngineProps extends EngineProps { | |
| readonly engineBatch: Batch; | ||
| readonly workerBatch: Batch; | ||
| readonly iops?: Size; | ||
| readonly vcpus?: number; | ||
| readonly engineMemoryMiB?: number; | ||
| } | ||
|
|
||
| const SNAKEMAKE_IMAGE_DESIGNATION = "snakemake"; | ||
|
|
||
| export class SnakemakeEngine extends Engine { | ||
| readonly headJobDefinition: JobDefinition; | ||
| private readonly volumeName = "efs"; | ||
| private readonly engineMemoryMiB = 4096; | ||
| public readonly fsap: AccessPoint; | ||
| public readonly fileSystem: FileSystem; | ||
|
|
||
|
|
@@ -41,7 +42,8 @@ export class SnakemakeEngine extends Engine { | |
| logGroup: this.logGroup, | ||
| platformCapabilities: [PlatformCapabilities.FARGATE], | ||
| container: { | ||
| memoryLimitMiB: this.engineMemoryMiB, | ||
| vcpus: props.vcpus || 1, | ||
| memoryLimitMiB: props.engineMemoryMiB || 4096, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you create a constant with a meaningful name for this magic number? |
||
| jobRole: engineBatch.role, | ||
| executionRole: engineBatch.role, | ||
| image: createEcrImage(this, SNAKEMAKE_IMAGE_DESIGNATION), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,6 +87,14 @@ export class ContextAppParameters { | |
| * The types of EC2 instances that may be launched in the compute environment. | ||
| */ | ||
| public readonly instanceTypes?: InstanceType[]; | ||
| /** | ||
| * The maximum number of Amazon EC2 vCPUs that an environment can reach for the nextflow engine. | ||
| */ | ||
| public readonly vCpus?: number; | ||
| /** | ||
| * The maximum memory limit that an environment can reach for the nextflow engine. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this NextFlow specific? How it will affect other supported engines? Have we tested this change with other engines?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, changes got updated to support all engines
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should you update comments than? |
||
| */ | ||
| public readonly memoryLimitMiB?: number; | ||
| /** | ||
| * If true, put EC2 instances into public subnets instead of private subnets. | ||
| * This allows you to obtain significantly lower ongoing costs if used in conjunction with the usePublicSubnets option | ||
|
|
@@ -133,6 +141,9 @@ export class ContextAppParameters { | |
| this.requestSpotInstances = getEnvBoolOrDefault(node, "REQUEST_SPOT_INSTANCES", false)!; | ||
| this.instanceTypes = instanceTypeStrings ? instanceTypeStrings.map((instanceType) => new InstanceType(instanceType.trim())) : undefined; | ||
|
|
||
| this.vCpus = getEnvNumber(node, "V_CPUS"); | ||
| this.memoryLimitMiB = getEnvNumber(node, "MEMORY_LIMIT_MIB"); | ||
|
|
||
| this.usePublicSubnets = getEnvBoolOrDefault(node, "PUBLIC_SUBNETS", false); | ||
| this.agcVersion = getEnvString(node, "AGC_VERSION"); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ import { CromwellEngineRole } from "../../roles/cromwell-engine-role"; | |
| import { CromwellAdapterRole } from "../../roles/cromwell-adapter-role"; | ||
| import { IJobQueue } from "@aws-cdk/aws-batch-alpha"; | ||
| import { Construct } from "constructs"; | ||
| import {ContextAppParameters} from "../../env"; | ||
|
|
||
| export interface CromwellEngineConstructProps extends EngineOptions { | ||
| /** | ||
|
|
@@ -57,7 +58,7 @@ export class CromwellEngineConstruct extends EngineConstruct { | |
| }); | ||
|
|
||
| // 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| this.adapterLogGroup = new LogGroup(this, "AdapterLogGroup"); | ||
|
|
||
| const lambda = this.renderAdapterLambda({ | ||
|
|
@@ -89,7 +90,13 @@ export class CromwellEngineConstruct extends EngineConstruct { | |
| }; | ||
| } | ||
|
|
||
| private getEngineServiceDefinition(vpc: IVpc, subnets: SubnetSelection, serviceContainer: ServiceContainer, logGroup: ILogGroup) { | ||
| private getEngineServiceDefinition( | ||
| vpc: IVpc, | ||
| subnets: SubnetSelection, | ||
| serviceContainer: ServiceContainer, | ||
| logGroup: ILogGroup, | ||
| params: ContextAppParameters | ||
| ) { | ||
| const id = "Engine"; | ||
| const fileSystem = new FileSystem(this, "EngineFileSystem", { | ||
| vpc, | ||
|
|
@@ -99,8 +106,8 @@ export class CromwellEngineConstruct extends EngineConstruct { | |
| }); | ||
| const definition = new FargateTaskDefinition(this, "EngineTaskDef", { | ||
| taskRole: this.engineRole, | ||
| cpu: serviceContainer.cpu, | ||
| memoryLimitMiB: serviceContainer.memoryLimitMiB, | ||
| cpu: (params.vCpus ? params.vCpus * 1024 : undefined) || serviceContainer.cpu, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the meaning of this |
||
| memoryLimitMiB: params.memoryLimitMiB || serviceContainer.memoryLimitMiB, | ||
| }); | ||
|
|
||
| const volumeName = "cromwell-executions"; | ||
|
|
@@ -112,8 +119,8 @@ export class CromwellEngineConstruct extends EngineConstruct { | |
| }); | ||
|
|
||
| const container = definition.addContainer(serviceContainer.serviceName, { | ||
| cpu: serviceContainer.cpu, | ||
| memoryLimitMiB: serviceContainer.memoryLimitMiB, | ||
| cpu: (params.vCpus ? params.vCpus * 1024 : undefined) || serviceContainer.cpu, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto. By the way, may be we should consider avoiding this duplicated code block? |
||
| memoryLimitMiB: params.memoryLimitMiB || serviceContainer.memoryLimitMiB, | ||
| environment: serviceContainer.environment, | ||
| containerName: serviceContainer.serviceName, | ||
| image: createEcrImage(this, serviceContainer.imageConfig.designation), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import { ToilJobRole } from "../../roles/toil-job-role"; | |
| import { ToilEngineRole } from "../../roles/toil-engine-role"; | ||
| import { IJobQueue } from "@aws-cdk/aws-batch-alpha"; | ||
| import { Construct } from "constructs"; | ||
| import { ContextAppParameters } from "../../env"; | ||
|
|
||
| export interface ToilEngineConstructProps extends EngineOptions { | ||
| /** | ||
|
|
@@ -55,7 +56,7 @@ export class ToilEngineConstruct extends EngineConstruct { | |
| TOIL_AWS_BATCH_JOB_ROLE_ARN: this.jobRole.roleArn, | ||
| }); | ||
|
|
||
| this.engine = this.getEngineServiceDefinition(props.vpc, props.subnets, engineContainer, this.engineLogGroup); | ||
| this.engine = this.getEngineServiceDefinition(props.vpc, props.subnets, engineContainer, this.engineLogGroup, params); | ||
|
|
||
| // We don't use an adapter, so put the access-controlling proxy right in | ||
| // front of the engine load balancer. | ||
|
|
@@ -74,17 +75,23 @@ export class ToilEngineConstruct extends EngineConstruct { | |
| }; | ||
| } | ||
|
|
||
| private getEngineServiceDefinition(vpc: IVpc, subnets: SubnetSelection, serviceContainer: ServiceContainer, logGroup: ILogGroup) { | ||
| private getEngineServiceDefinition( | ||
| vpc: IVpc, | ||
| subnets: SubnetSelection, | ||
| serviceContainer: ServiceContainer, | ||
| logGroup: ILogGroup, | ||
| params: ContextAppParameters | ||
| ) { | ||
| const id = "Engine"; | ||
| const definition = new FargateTaskDefinition(this, "EngineTaskDef", { | ||
| taskRole: this.engineRole, | ||
| cpu: serviceContainer.cpu, | ||
| memoryLimitMiB: serviceContainer.memoryLimitMiB, | ||
| cpu: (params.vCpus ? params.vCpus * 1024 : undefined) || serviceContainer.cpu, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we go again... I think if some number has to be repeated more than once, it should be moved to a constant. |
||
| memoryLimitMiB: params.memoryLimitMiB || serviceContainer.memoryLimitMiB, | ||
| }); | ||
|
|
||
| definition.addContainer(serviceContainer.serviceName, { | ||
| cpu: serviceContainer.cpu, | ||
| memoryLimitMiB: serviceContainer.memoryLimitMiB, | ||
| cpu: (params.vCpus ? params.vCpus * 1024 : undefined) || serviceContainer.cpu, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and one more time... |
||
| memoryLimitMiB: params.memoryLimitMiB || serviceContainer.memoryLimitMiB, | ||
| environment: serviceContainer.environment, | ||
| containerName: serviceContainer.serviceName, | ||
| image: createEcrImage(this, serviceContainer.imageConfig.designation), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,10 +10,17 @@ type Filesystem struct { | |
| Configuration FSConfig `yaml:"configuration,omitempty"` | ||
| } | ||
| type Engine struct { | ||
| Type string `yaml:"type"` | ||
| Engine string `yaml:"engine"` | ||
| Filesystem Filesystem `yaml:"filesystem,omitempty"` | ||
| Type string `yaml:"type"` | ||
| Engine string `yaml:"engine"` | ||
| ResourceRequirements ResourceRequirement `yaml:"resourceRequirements,omitempty"` | ||
| Filesystem Filesystem `yaml:"filesystem,omitempty"` | ||
| } | ||
|
|
||
| type ResourceRequirement struct { | ||
| VCpus int `yaml:"vcpus,omitempty"` | ||
| MemoryLimitMiB int `yaml:"memoryLimit,omitempty"` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest a unit test for that. |
||
| } | ||
|
|
||
| type Context struct { | ||
| InstanceTypes []string `yaml:"instanceTypes,omitempty"` | ||
| RequestSpotInstances bool `yaml:"requestSpotInstances,omitempty"` | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,6 +110,10 @@ schemaVersion: 0 | |
| { | ||
| Type: "nextflow", | ||
| Engine: "nextflow", | ||
| ResourceRequirements: ResourceRequirement{ | ||
| VCpus: 2, | ||
| MemoryLimitMiB: 4048, | ||
| }, | ||
| Filesystem: Filesystem{ | ||
| FSType: "S3", | ||
| }, | ||
|
|
@@ -122,6 +126,10 @@ schemaVersion: 0 | |
| { | ||
| Type: "nextflow", | ||
| Engine: "nextflow", | ||
| ResourceRequirements: ResourceRequirement{ | ||
| VCpus: 2, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets add tests with partial configuration. e.g. either VCpus or MemoryLimitMiB is set, but not both. |
||
| MemoryLimitMiB: 4048, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
|
|
@@ -161,13 +169,19 @@ contexts: | |
| engines: | ||
| - type: nextflow | ||
| engine: nextflow | ||
| resourceRequirements: | ||
| vcpus: 2 | ||
| memoryLimit: 4048 | ||
| filesystem: | ||
| fsType: S3 | ||
| ctx3: | ||
| maxVCpus: 256 | ||
| engines: | ||
| - type: nextflow | ||
| engine: nextflow | ||
| resourceRequirements: | ||
| vcpus: 2 | ||
| memoryLimit: 4048 | ||
| `, | ||
| }, | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you create a constant with a meaningful name for this magic number?