Add extra large scheduler size option #1696

neel-astro · 2024-08-02T17:35:07Z

Description

This PR contains changes to allow the user to pass extra_large as a scheduler size option when creating/updating Hosted deployments either via CLI command or via YAML file

🎟 Issue(s)

Related #22568

🧪 Functional Testing

List the functional testing steps to confirm this feature or fix.

Tested Extra Large deployment creation and update via CLI command and via YAML file, attached screenshot for reference

📸 Screenshots

Add screenshots to illustrate the validity of these changes.

📋 Checklist

Rebased from the main (or release if patching) branch (before testing)
Ran make test before taking out of draft
Ran make lint before taking out of draft
Added/updated applicable tests
Tested against Astro-API (if necessary).
Tested against Houston-API and Astronomer (if necessary).
Communicated to/tagged owners of respective clients potentially impacted by these changes.
Updated any related documentation

codecov · 2024-08-02T17:35:23Z

Codecov Report

Attention: Patch coverage is 46.66667% with 16 lines in your changes missing coverage. Please review.

Project coverage is 86.21%. Comparing base (be4ca15) to head (a1f39ae).

Files	Patch %	Lines
cloud/deployment/fromfile/fromfile.go	30.00%	14 Missing ⚠️
cloud/deployment/deployment.go	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1696      +/-   ##
==========================================
- Coverage   86.24%   86.21%   -0.04%     
==========================================
  Files         117      117              
  Lines       17095    17111      +16     
==========================================
+ Hits        14744    14752       +8     
- Misses       1417     1425       +8     
  Partials      934      934

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ianbuss

Small comments

ianbuss · 2024-08-05T08:57:38Z

cloud/deployment/deployment_test.go

@@ -978,6 +978,10 @@ func (s *Suite) TestCreate() {
 		err = Create("test-name", ws, "test-desc", csID, "4.2.5", dagDeploy, CeleryExecutor, "", "", LargeScheduler, "", "", "", "", "", "", "", "", astroplatformcore.DeploymentTypeHYBRID, 0, 0, mockPlatformCoreClient, mockCoreClient, false)
 		s.NoError(err)

+		// Call the Create function
+		err = Create("test-name", ws, "test-desc", csID, "4.2.5", dagDeploy, CeleryExecutor, "", "", ExtraLargeScheduler, "", "", "", "", "", "", "", "", astroplatformcore.DeploymentTypeHYBRID, 0, 0, mockPlatformCoreClient, mockCoreClient, false)


Should these tests use a hosted deployment type, since scheduler size is not handled with t-shirt sizes in hybrid?

😬 good catch

cmd/cloud/deployment.go

ianbuss · 2024-08-05T09:06:33Z

cloud/deployment/fromfile/fromfile.go

@@ -327,12 +327,14 @@ func createOrUpdateDeployment(deploymentFromFile *inspect.FormattedDeployment, c

 			var schedulerSize astroplatformcore.CreateStandardDeploymentRequestSchedulerSize
 			switch strings.ToUpper(deploymentFromFile.Deployment.Configuration.SchedulerSize) {
-			case deployment.SMALL:
+			case string(astrocore.CreateStandardDeploymentRequestSchedulerSizeSMALL):


Should we just do:

switch deploymentFromFile.Deployment.Configuration.SchedulerSize: case deployment.SmallScheduler: ...

?

it's a case of "small" vs "SMALL" 🙈, we are taking "small" as user input in CLI and API expects "SMALL"

So:

switch strings.ToLower(deploymentFromFile.Deployment.Configuration.SchedulerSize): case deployment.SmallScheduler:

?

I might prefer relying on the auto-generated enums from the Core API client package, for comparing against the Core API client response, just to avoid any unwanted harm down the line. But I agree the wording is way too verbose.

I like driving this from the (indeed verbose) client constants too. Should we then also do this in the deployment package and remove the *Scheduler constants?

It's a shame the client doesn't also provide an enumeration of the constants so that we wouldn't need to refer to individual values in the CLI at all, including the flag description. We could bake something up with the reflect package based on the common prefix but perhaps that's getting too clever.

you mean the smallScheduler, mediumScheduler, largeScheduler, .... I think we might need to live with them because they represent the naming convention (lowercase) that the CLI expects as input for the scheduler size field (it's a case of CLI expecting "small" as input vs API expecting "SMALL" as input 🙂 ).

I am divided if we should just use strings.ToLower(<CORE API CLIENT CONSTANT>), let me know if you feel strongly about it.

I'd expect that since we already removed deployment.SMALL etc we'd also remove smallScheduler etc. Yeah it's verbose with the strings.ToLower added on but it's closer to the truth of what is a valid input than redefining it as a constant.

updated the logic

ianbuss · 2024-08-05T11:01:09Z

cloud/deployment/deployment.go

@@ -945,6 +947,8 @@ func Update(deploymentID, name, ws, description, deploymentName, dagDeploy, exec
 				standardDeploymentRequest.SchedulerSize = astroplatformcore.UpdateStandardDeploymentRequestSchedulerSizeMEDIUM
 			case LargeScheduler:
 				standardDeploymentRequest.SchedulerSize = astroplatformcore.UpdateStandardDeploymentRequestSchedulerSizeLARGE
+			case ExtraLargeScheduler:
+				standardDeploymentRequest.SchedulerSize = astroplatformcore.UpdateStandardDeploymentRequestSchedulerSizeEXTRALARGE


Should we sprinkle a standard deployment test in to make this warning go away?

added the unit test 🙃

cmd/cloud/deployment.go

ianbuss

If you're happy the warnings about code coverage are tested adequately elsewhere, this lgtm.

jeremybeard · 2024-08-05T15:54:45Z

cloud/deployment/fromfile/fromfile.go

@@ -327,12 +327,14 @@ func createOrUpdateDeployment(deploymentFromFile *inspect.FormattedDeployment, c

 			var schedulerSize astroplatformcore.CreateStandardDeploymentRequestSchedulerSize
 			switch strings.ToUpper(deploymentFromFile.Deployment.Configuration.SchedulerSize) {
-			case deployment.SMALL:
+			case string(astrocore.CreateStandardDeploymentRequestSchedulerSizeSMALL):


I like driving this from the (indeed verbose) client constants too. Should we then also do this in the deployment package and remove the *Scheduler constants?

It's a shame the client doesn't also provide an enumeration of the constants so that we wouldn't need to refer to individual values in the CLI at all, including the flag description. We could bake something up with the reflect package based on the common prefix but perhaps that's getting too clever.

cloud/deployment/deployment_test.go

for more information, see https://pre-commit.ci

neel-astro added 2 commits August 2, 2024 20:45

add extra large scheduler size option

43f2812

fix linters

e43ca14

neel-astro requested review from jeremybeard, kushalmalani and sunkickr as code owners August 2, 2024 17:35

neel-astro requested review from ianbuss and pritt20 August 5, 2024 05:44

ianbuss reviewed Aug 5, 2024

View reviewed changes

fix unit tests to use hosted deployment type

6710c51

ianbuss reviewed Aug 5, 2024

View reviewed changes

ianbuss approved these changes Aug 5, 2024

View reviewed changes

add unit tests for standard deployment

a1f39ae

jeremybeard reviewed Aug 7, 2024

View reviewed changes

move away from defining scheduler size constant in CLI

7d9c066

jeremybeard approved these changes Aug 8, 2024

View reviewed changes

neel-astro and others added 2 commits August 8, 2024 20:25

Merge branch 'main' into extra-large-deployment

1f9de3b

[pre-commit.ci] auto fixes from pre-commit.com hooks

e5ff8c1

for more information, see https://pre-commit.ci

neel-astro merged commit 63c58bc into main Aug 8, 2024
3 checks passed

neel-astro deleted the extra-large-deployment branch August 8, 2024 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add extra large scheduler size option #1696

Add extra large scheduler size option #1696

neel-astro commented Aug 2, 2024 •

edited

Loading

codecov bot commented Aug 2, 2024 •

edited

Loading

ianbuss left a comment

ianbuss Aug 5, 2024

neel-astro Aug 5, 2024

ianbuss Aug 5, 2024

neel-astro Aug 5, 2024

ianbuss Aug 5, 2024

neel-astro Aug 5, 2024

jeremybeard Aug 5, 2024

neel-astro Aug 7, 2024

jeremybeard Aug 7, 2024

neel-astro Aug 8, 2024

ianbuss Aug 5, 2024

neel-astro Aug 5, 2024

ianbuss left a comment

jeremybeard Aug 5, 2024

Add extra large scheduler size option #1696

Add extra large scheduler size option #1696

Conversation

neel-astro commented Aug 2, 2024 • edited Loading

Description

🎟 Issue(s)

🧪 Functional Testing

📸 Screenshots

📋 Checklist

codecov bot commented Aug 2, 2024 • edited Loading

Codecov Report

ianbuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianbuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neel-astro commented Aug 2, 2024 •

edited

Loading

codecov bot commented Aug 2, 2024 •

edited

Loading