Skip to content
This repository was archived by the owner on May 31, 2024. It is now read-only.

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

Open
josuebc wants to merge 7 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e6ad46a
Feat: Adding the option to provide cpu count and memory size for the …
josuebc Apr 18, 2022
df06b26
Feat: Adding the option to provide cpu count and memory size for the …
josuebc Apr 18, 2022
eeda242
Feat: Adding the option to provide cpu count and memory size for the …
josuebc Apr 18, 2022
48e327c
Feat: Adding the option to provide cpu count and memory size for the …
josuebc Apr 18, 2022
e723969
Merge branch 'main' into josueba/nextflowConfigEngineSize
josuebc May 18, 2022
4c9ad65
Feat: Adding the option to provide cpu count and memory size for the …
josuebc May 25, 2022
7d28f6b
Update site/content/en/docs/Concepts/contexts.md
josuebc May 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions examples/demo-nextflow-project/agc-project.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,6 @@ contexts:
engines:
- type: nextflow
engine: nextflow
resourceRequirements:
vcpus: 1
memoryLimit: 2048
4 changes: 4 additions & 0 deletions packages/cdk/lib/constructs/engines/nextflow/nextflow-engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import { Engine, EngineProps } from "../engine";
export interface NextflowEngineProps extends EngineProps {
readonly jobQueueArn: string;
readonly taskRole: IRole;
readonly vcpus?: number;
readonly engineMemoryMiB?: number;
}

const NEXTFLOW_IMAGE_DESIGNATION = "nextflow";
Expand All @@ -21,6 +23,8 @@ export class NextflowEngine extends Engine {
this.headJobDefinition = new EngineJobDefinition(this, "NexflowHeadJobDef", {
logGroup: this.logGroup,
container: {
vcpus: props.vcpus,
memoryLimitMiB: props.engineMemoryMiB,
jobRole: props.taskRole,
image: createEcrImage(this, NEXTFLOW_IMAGE_DESIGNATION),
command: [],
Expand Down
11 changes: 11 additions & 0 deletions packages/cdk/lib/env/context-app-parameters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

@IllyaYalovyy IllyaYalovyy Apr 27, 2022

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

@josuebc josuebc May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@IllyaYalovyy IllyaYalovyy Jun 3, 2022

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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");

Expand Down
2 changes: 2 additions & 0 deletions packages/cdk/lib/stacks/engines/nextflow-engine-construct.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ export class NextflowEngineConstruct extends EngineConstruct {
});

this.nextflowEngine = new NextflowEngine(this, "NextflowEngine", {
vcpus: params.vCpus,
engineMemoryMiB: params.memoryLimitMiB,
vpc: props.vpc,
jobQueueArn: props.jobQueue.jobQueueArn,
rootDirS3Uri: params.getEngineBucketPath(),
Expand Down
4 changes: 4 additions & 0 deletions packages/cli/internal/pkg/cli/context/context_environment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ type contextEnvironment struct {
ResourceType string
MaxVCpus int

VCpus int
MemoryLimitMiB int
RequestSpotInstances bool
UsePublicSubnets bool
}
Expand Down Expand Up @@ -68,5 +70,7 @@ func (input contextEnvironment) ToEnvironmentList() []string {
"MAX_V_CPUS": strconv.Itoa(input.MaxVCpus),
"REQUEST_SPOT_INSTANCES": strconv.FormatBool(input.RequestSpotInstances),
"PUBLIC_SUBNETS": strconv.FormatBool(input.UsePublicSubnets),
"V_CPUS": strconv.Itoa(input.VCpus),
"MEMORY_LIMIT_MIB": strconv.Itoa(input.MemoryLimitMiB),
})
}
2 changes: 2 additions & 0 deletions packages/cli/internal/pkg/cli/context/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,8 @@ func (m *Manager) setContextEnv(contextName string) {
ReadWriteBucketArns: strings.Join(m.readWriteBuckets, listDelimiter),
InstanceTypes: strings.Join(m.contextSpec.InstanceTypes, listDelimiter),
MaxVCpus: m.contextSpec.MaxVCpus,
VCpus: context.Engines[0].ResourceRequirements.VCpus,
MemoryLimitMiB: context.Engines[0].ResourceRequirements.MemoryLimitMiB,
RequestSpotInstances: m.contextSpec.RequestSpotInstances,
UsePublicSubnets: m.contextSpec.UsePublicSubnets,
// TODO: we default to a single engine in a context for now
Expand Down
10 changes: 5 additions & 5 deletions packages/cli/internal/pkg/cli/context/manager_deploy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func TestManager_Deploy(t *testing.T) {
mockClients.ssmMock.EXPECT().GetCustomTags().Return(testTags)
mockClients.ecrClientMock.EXPECT().VerifyImageExists(environment.CommonImages["NEXTFLOW"]).Return(nil)
clearContext := mockClients.cdkMock.EXPECT().ClearContext(filepath.Join(testHomeDir, ".agc/cdk/apps/context")).Return(nil)
mockClients.cdkMock.EXPECT().DeployApp(filepath.Join(testHomeDir, ".agc/cdk/apps/context"), gomock.Len(43), testContextName3).After(clearContext).Return(mockClients.progressStream1, nil)
mockClients.cdkMock.EXPECT().DeployApp(filepath.Join(testHomeDir, ".agc/cdk/apps/context"), gomock.Len(45), testContextName3).After(clearContext).Return(mockClients.progressStream1, nil)
displayProgressBar = mockClients.cdkMock.DisplayProgressBar
mockClients.cdkMock.EXPECT().DisplayProgressBar(fmt.Sprintf("Deploying resources for context(s) %s", []string{testContextName3}), []cdk.ProgressStream{mockClients.progressStream1}).Return([]cdk.Result{{Outputs: []string{"some message"}, ExecutionName: testContextName3}})
return mockClients
Expand All @@ -69,7 +69,7 @@ func TestManager_Deploy(t *testing.T) {
mockClients.ssmMock.EXPECT().GetCustomTags().Return("")
mockClients.ecrClientMock.EXPECT().VerifyImageExists(environment.CommonImages["NEXTFLOW"]).Return(nil)
clearContext := mockClients.cdkMock.EXPECT().ClearContext(filepath.Join(testHomeDir, ".agc/cdk/apps/context")).Return(nil)
mockClients.cdkMock.EXPECT().DeployApp(filepath.Join(testHomeDir, ".agc/cdk/apps/context"), gomock.Len(43), testContextName3).After(clearContext).Return(mockClients.progressStream1, nil)
mockClients.cdkMock.EXPECT().DeployApp(filepath.Join(testHomeDir, ".agc/cdk/apps/context"), gomock.Len(45), testContextName3).After(clearContext).Return(mockClients.progressStream1, nil)
displayProgressBar = mockClients.cdkMock.DisplayProgressBar
mockClients.cdkMock.EXPECT().DisplayProgressBar(fmt.Sprintf("Deploying resources for context(s) %s", []string{testContextName3}), []cdk.ProgressStream{mockClients.progressStream1}).Return([]cdk.Result{{Outputs: []string{"some message"}, ExecutionName: testContextName3}})
return mockClients
Expand All @@ -94,8 +94,8 @@ func TestManager_Deploy(t *testing.T) {
mockClients.ecrClientMock.EXPECT().VerifyImageExists(environment.CommonImages["CROMWELL"]).Times(2).Return(nil)
clearContext := mockClients.cdkMock.EXPECT().ClearContext(filepath.Join(testHomeDir, ".agc/cdk/apps/context")).Return(nil)
clearContext2 := mockClients.cdkMock.EXPECT().ClearContext(filepath.Join(testHomeDir, ".agc/cdk/apps/context")).Return(nil)
mockClients.cdkMock.EXPECT().DeployApp(filepath.Join(testHomeDir, ".agc/cdk/apps/context"), gomock.Len(43), testContextName1).After(clearContext).Return(mockClients.progressStream1, nil)
mockClients.cdkMock.EXPECT().DeployApp(filepath.Join(testHomeDir, ".agc/cdk/apps/context"), gomock.Len(43), testContextName2).After(clearContext2).Return(mockClients.progressStream2, nil)
mockClients.cdkMock.EXPECT().DeployApp(filepath.Join(testHomeDir, ".agc/cdk/apps/context"), gomock.Len(45), testContextName1).After(clearContext).Return(mockClients.progressStream1, nil)
mockClients.cdkMock.EXPECT().DeployApp(filepath.Join(testHomeDir, ".agc/cdk/apps/context"), gomock.Len(45), testContextName2).After(clearContext2).Return(mockClients.progressStream2, nil)
displayProgressBar = mockClients.cdkMock.DisplayProgressBar
expectedCdkResult := []cdk.Result{{Outputs: []string{"some message"}, ExecutionName: testContextName1}, {Outputs: []string{"some other message"}, ExecutionName: testContextName2}}
mockClients.cdkMock.EXPECT().DisplayProgressBar(fmt.Sprintf("Deploying resources for context(s) %s", []string{testContextName1, testContextName2}), []cdk.ProgressStream{mockClients.progressStream1, mockClients.progressStream2}).Return(expectedCdkResult)
Expand Down Expand Up @@ -235,7 +235,7 @@ func TestManager_Deploy(t *testing.T) {
mockClients.ssmMock.EXPECT().GetCommonParameter("installed-artifacts/s3-root-url").Return(testArtifactBucket, nil)
mockClients.ssmMock.EXPECT().GetCustomTags().Return(testTags)
mockClients.cdkMock.EXPECT().ClearContext(filepath.Join(testHomeDir, ".agc/cdk/apps/context")).Return(nil)
mockClients.cdkMock.EXPECT().DeployApp(filepath.Join(testHomeDir, ".agc/cdk/apps/context"), gomock.Len(43), testContextName1).Return(nil, fmt.Errorf("some context error"))
mockClients.cdkMock.EXPECT().DeployApp(filepath.Join(testHomeDir, ".agc/cdk/apps/context"), gomock.Len(45), testContextName1).Return(nil, fmt.Errorf("some context error"))
mockClients.ecrClientMock.EXPECT().VerifyImageExists(environment.CommonImages["CROMWELL"]).Return(nil)
return mockClients
},
Expand Down
13 changes: 10 additions & 3 deletions packages/cli/internal/pkg/cli/spec/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Copy link
Contributor

@IllyaYalovyy IllyaYalovyy Jun 3, 2022

Choose a reason for hiding this comment

The 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:
https://go.dev/play/p/WB4AzY1JhTT

Copy link
Contributor

@IllyaYalovyy IllyaYalovyy Jun 3, 2022

Choose a reason for hiding this comment

The 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"`
Expand Down
15 changes: 15 additions & 0 deletions packages/cli/internal/pkg/cli/spec/project_schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,21 @@
"type": "string",
"minLength": 1
},
"resourceRequirements": {
"type": "object",
"items": {
"properties": {
"vcpus":{
"type":"integer",
"minimum": 1
},
"memoryLimit":{
"type":"integer",
"minimum": 1
}
}
}
},
"filesystem": {
"type": "object",
"items": {
Expand Down
14 changes: 14 additions & 0 deletions packages/cli/internal/pkg/cli/spec/project_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,10 @@ schemaVersion: 0
{
Type: "nextflow",
Engine: "nextflow",
ResourceRequirements: ResourceRequirement{
VCpus: 2,
MemoryLimitMiB: 4048,
},
Filesystem: Filesystem{
FSType: "S3",
},
Expand All @@ -122,6 +126,10 @@ schemaVersion: 0
{
Type: "nextflow",
Engine: "nextflow",
ResourceRequirements: ResourceRequirement{
VCpus: 2,
Copy link
Contributor

@IllyaYalovyy IllyaYalovyy Jun 3, 2022

Choose a reason for hiding this comment

The 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,
},
},
},
},
Expand Down Expand Up @@ -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
`,
},
}
Expand Down
18 changes: 18 additions & 0 deletions site/content/en/docs/Concepts/contexts.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,25 @@ contexts:
- type: nextflow
engine: nextflow
```
### Engine vCpus and Memory
*default minimums:*
*cpus:* 1
*memory:* 2048

You may optionally specify the minimum number of vCpus and memory used in a context job.

*note:* if these are not provided the defaults would be used.
```yaml
contexts:
spotContext:
requestSpotInstances: true
engines:
- type: nextflow
engine: nextflow
resourceRequirements:
vcpus: 3
memoryLimit: 6144
```
### Public Subnets

In the interest of saving money, in particular if you intend to have the AGC stack deployed for a long period, you may choose to deploy in "public subnet" mode.
Expand Down
15 changes: 15 additions & 0 deletions site/content/en/docs/Concepts/engines.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,21 @@ contexts:
engine: cromwell
```

The following snippet
defines a Nextflow DSL engine of type `Nextflow` as part of the context named `spotContext` with a minimum requirement count of Cpus and Memory, when these are not provided the defaults would be used. (cpus:1, memory: 2048)

```yaml
contexts:
spotContext:
requestSpotInstances: true
engines:
- type: nextflow
engine: nextflow
resourceRequirements:
vcpus: 1
memoryLimit: 2048
```

## Commands

There are no commands specific to engines. Engines are [deployed]( {{< relref "contexts#deploy" >}} ) along with contexts by the [`context` commands]( {{< relref "contexts#context-commands" >}} ) and workflows
Expand Down