Bugfix: Fix pyfunc ensembler undeployment bug #331

deadlycoconuts · 2023-04-12T07:39:51Z

Context

After the Gorm library has been updated to v2 in #261, there is a bug discovered in how we're using the Gorm methods to save router versions to the DB whereby nested fields (such as the ensembler configs) do not get updated in their respective tables. This has caused deployed pyfunc ensemblers to not have their corresponding docker configs updated in the ensembler_configs table. Since these are left as NULL, what then happens downstream is that the cluster controller fails to build KnativeService objects that correspond to the deployed ensembler instances, causing it to be unable to deploy ensemblers that have been deployed (either as part of a router update operation or an undeployment operation).

This PR introduces a config to the Gorm DB layer which adds a new field FullSaveAssociations: true to the existing Gorm DB configs, which would ensure that all tables get updated whenever Gorm is used to update a router version (or any other object for that matter of fact). See go-gorm/gorm#6162 for more details.

This PR introduces additional steps to update the ensembler and enricher configs of a router version (these are stored in their respective tables) and were previously not getting updated whenever a router version is updated:

err = tx.Save(routerVersion).Error
if err != nil {
	return nil, err
}

if routerVersion.Enricher != nil {
	err = tx.Save(routerVersion.Enricher).Error
	if err != nil {
		return nil, err
	}
}
if routerVersion.Ensembler != nil {
	err = tx.Save(routerVersion.Ensembler).Error
	if err != nil {
		return nil, err
	}
}

Totally Unrelated Changes - Unused Arguments Clean Up

The linter's raising a lot of warnings about unused arguments so I've decided to remove them completely wherever possible, which is the case for the methods like NewRouterEndpoint, NewEnricherService and NewEnsemblerService since the existing implementations of the the clusterSvcBuilder don't even use them. And since these methods are concrete implementations of the interface ClusterServiceBuilder's methods, I've also removed the arguments from them too, which then requires some minor refactoring of the methods of mockClusterServiceBuilder (which also implements the interface) and the tests where it is used.

Most notably, I've removed all instances of the following:

			Labels: map[string]string{
				"env": envType,
			},

that's specified in the expected and mocked responses (in api/turing/service/router_deployment_service_test.go) since in no way is the actual implementation of the clusterSvcBuilder ever using the envType argument to set labels in the KnativeService struct returned. It's weird to continue mocking responses that set those labels and then configure the expected responses to include those labels.

In all other cases (the majority of them), I've simply renamed the remaining unused arguments to _ so those should be pretty easy to look through.

deadlycoconuts · 2023-04-12T09:33:09Z

api/turing/service/router_deployment_service_test.go

-			Labels: map[string]string{
-				"env": envType,
-			},


This should no longer be here the actual clusterServiceBuilder does not even use an argument envType to set the labels.

deadlycoconuts · 2023-04-12T09:35:22Z

api/turing/service/router_deployment_service_test.go

-			Labels: map[string]string{
-				"env": envType,
-			},


Removed these from the expected responses since we were just 'expecting' a mocked response though in reality the actual method doesn't even actually set the envType as the env label.

krithika369

Thanks for tracking this down, @deadlycoconuts ! Left some questions.

krithika369 · 2023-04-12T23:46:51Z

api/turing/service/router_deployment_service.go

+		Endpoint:          PyFuncEnsemblerServiceEndpoint,
+		Port:              PyFuncEnsemblerServicePort,
+		Env:               pyfuncConfig.Env,
+		ServiceAccount:    "",


Nit: If this is going to be an empty value, we can just leave this field unset.

Thanks for pointing that out, I've just removed that :)

api/turing/service/router_deployment_service.go

…us is pending

deadlycoconuts · 2023-04-17T09:54:28Z

api/e2e/test/router_e2e_test.go

@@ -184,7 +184,6 @@ var _ = DeployedRouterContext("testdata/create_router_nop_logger_proprietary_exp

 							Eventually(func(g Gomega) {
 								router = api.GetRouter(apiE, routerCtx.ProjectID, routerCtx.ID)
-								// TODO: Why is it pending? - g.Expect(router.Value("status").Raw()).To(Equal(api.Status.Pending))


Removed this as it is no longer needed (I agree that we shouldn't be expecting the pending state).

deadlycoconuts · 2023-04-17T10:09:34Z

sdk/e2e/01_create_router_test.py

        ),
-        endpoint="anything",
+        endpoint="ensemble",


Made these changes to the SDK tests so these configs resemble the test configs here: https://github.com/caraml-dev/turing/blob/main/api/e2e/test/testdata/create_router_nop_logger_proprietary_exp.json.tmpl.

deadlycoconuts · 2023-04-17T10:10:43Z

sdk/e2e/06_deploy_valid_config_test.py

+    # check that the status of the router is pending
+    try:
+        router.wait_for_status(RouterStatus.PENDING)
+    except TimeoutError:
+        raise Exception(
+            f"Turing API is taking too long for router {router.id} to start getting deployed."
+        )


Added this block here in order to verify that the router enters a pending state before being deployed. This is similar to the API e2e test here: https://github.com/caraml-dev/turing/blob/main/api/e2e/test/router_e2e_test.go#L236.

krithika369

LGTM. Thanks for sorting this out and all the test case analysis, @deadlycoconuts !

deadlycoconuts added the type: bug Something isn't working label Apr 12, 2023

deadlycoconuts self-assigned this Apr 12, 2023

deadlycoconuts marked this pull request as ready for review April 12, 2023 08:22

deadlycoconuts requested review from a team and ariefrahmansyah April 12, 2023 08:22

deadlycoconuts commented Apr 12, 2023

View reviewed changes

krithika369 approved these changes Apr 13, 2023

View reviewed changes

deadlycoconuts force-pushed the fix_pyfunc_ensembler_undeployment_bug branch from ae645a6 to 8a71e04 Compare April 13, 2023 09:49

deadlycoconuts mentioned this pull request Apr 13, 2023

Add gorm config to update nested fields caraml-dev/xp#70

Merged

deadlycoconuts force-pushed the fix_pyfunc_ensembler_undeployment_bug branch 2 times, most recently from 4404564 to 973bce2 Compare April 14, 2023 03:06

deadlycoconuts added 11 commits April 14, 2023 13:05

Remove redundant error handling

86ac3d6

Remove unused envType argument from NewRouterEndpoint

5502f31

Remove unsused secretName argument in buildRouterEnvs

1967ec8

Remove unused envType argument from NewEnricherService

df1ca0c

Remove unused envType argument from NewEnsemblerService

78380d3

Rename unused request argument in experiments api methods

7570dbb

Rename unused arguments in routers api

f4274a4

Rename unused arguments in all remaining api files

c1a7a4f

Rename unused arguments in router deployment service test file

153198d

Remove obsolete labels from mocked and expected responses

e8f630a

Remove redundant fields

5941b80

deadlycoconuts force-pushed the fix_pyfunc_ensembler_undeployment_bug branch 3 times, most recently from 239fdac to dbfdac5 Compare April 17, 2023 08:09

deadlycoconuts added 2 commits April 17, 2023 17:04

Align sdk e2e test configs with those in api tests

d33f16d

Add additional steps to update enricher and ensemblers separately

64c2cbf

deadlycoconuts force-pushed the fix_pyfunc_ensembler_undeployment_bug branch from dbfdac5 to 64c2cbf Compare April 17, 2023 09:07

Remove obsolete todo comment

898c306

deadlycoconuts added 2 commits April 17, 2023 17:14

Add an additional step to sdk e2e test to ensure that the router stat…

c379bc1

…us is pending

Add additional checks before saving enrichers and ensemblers to db

d1da572

deadlycoconuts requested a review from krithika369 April 17, 2023 09:53

deadlycoconuts commented Apr 17, 2023

View reviewed changes

deadlycoconuts mentioned this pull request Apr 17, 2023

Revert PR (#70) - addition of gorm config to update nested fields in the db caraml-dev/xp#71

Merged

deadlycoconuts commented Apr 17, 2023

View reviewed changes

krithika369 approved these changes Apr 18, 2023

View reviewed changes

deadlycoconuts merged commit 6c7b8be into caraml-dev:main Apr 18, 2023

deadlycoconuts deleted the fix_pyfunc_ensembler_undeployment_bug branch June 6, 2023 07:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: Fix pyfunc ensembler undeployment bug #331

Bugfix: Fix pyfunc ensembler undeployment bug #331

deadlycoconuts commented Apr 12, 2023 •

edited

Loading

deadlycoconuts Apr 12, 2023

deadlycoconuts Apr 12, 2023

krithika369 left a comment

krithika369 Apr 12, 2023

deadlycoconuts Apr 13, 2023

deadlycoconuts Apr 17, 2023

deadlycoconuts Apr 17, 2023

deadlycoconuts Apr 17, 2023

krithika369 left a comment

Bugfix: Fix pyfunc ensembler undeployment bug #331

Bugfix: Fix pyfunc ensembler undeployment bug #331

Conversation

deadlycoconuts commented Apr 12, 2023 • edited Loading

Context

Totally Unrelated Changes - Unused Arguments Clean Up

deadlycoconuts Apr 12, 2023

Choose a reason for hiding this comment

deadlycoconuts Apr 12, 2023

Choose a reason for hiding this comment

krithika369 left a comment

Choose a reason for hiding this comment

krithika369 Apr 12, 2023

Choose a reason for hiding this comment

deadlycoconuts Apr 13, 2023

Choose a reason for hiding this comment

deadlycoconuts Apr 17, 2023

Choose a reason for hiding this comment

deadlycoconuts Apr 17, 2023

Choose a reason for hiding this comment

deadlycoconuts Apr 17, 2023

Choose a reason for hiding this comment

krithika369 left a comment

Choose a reason for hiding this comment

deadlycoconuts commented Apr 12, 2023 •

edited

Loading