From eaac564ebd063f315645345b32677461407f04a4 Mon Sep 17 00:00:00 2001 From: Michael Haas Date: Fri, 17 Sep 2021 09:19:25 +0200 Subject: [PATCH 1/3] Revert: pass external job_id and deployment_id The ability to pass an external job_id and deployment_id never materialized in the Data Attribute Recommendation service. For this reason, this commit removes the corresponding functionality in the SDK. Closes #106 --- sap/aibus/dar/client/model_manager_client.py | 48 +----- .../dar/client/test_model_manager_client.py | 148 +----------------- 2 files changed, 4 insertions(+), 192 deletions(-) diff --git a/sap/aibus/dar/client/model_manager_client.py b/sap/aibus/dar/client/model_manager_client.py index fc219b5..5499c1f 100644 --- a/sap/aibus/dar/client/model_manager_client.py +++ b/sap/aibus/dar/client/model_manager_client.py @@ -6,7 +6,6 @@ import typing -from uuid import UUID from sap.aibus.dar.client.base_client import BaseClientWithSession from sap.aibus.dar.client.exceptions import ( @@ -136,7 +135,6 @@ def create_job( model_name: str, dataset_id: str, model_template_id: str, - job_id: typing.Union[str, UUID] = None, ) -> dict: """ Creates a training Job. @@ -151,17 +149,9 @@ def create_job( A convenience method is available at :meth:`create_job_and_wait` which will submit a job and wait for its completion. - The *job_id* parameter is optional and can be used to specify the ID of the - newly created job. It must be a UUID. - - .. note :: - - The functionality to override the Job ID is not generally available. - :param model_name: Name of the model to train :param dataset_id: Id of previously uploaded, valid dataset :param model_template_id: Model template ID for training - :param job_id: Optionally provide job UUID :return: newly created Job as dict """ self.log.info( @@ -176,9 +166,6 @@ def create_job( "datasetId": dataset_id, "modelTemplateId": model_template_id, } - if job_id: - self.log.info("Job ID override specified: %s", job_id) - payload["id"] = str(job_id) response = self.session.post_to_endpoint( ModelManagerPaths.ENDPOINT_JOB_COLLECTION, payload=payload ) @@ -192,7 +179,6 @@ def create_job_and_wait( model_name: str, dataset_id: str, model_template_id: str, - job_id: typing.Union[str, UUID] = None, ): """ Starts a job and waits for the job to finish. @@ -200,17 +186,9 @@ def create_job_and_wait( This method is a thin wrapper around :meth:`create_job` and :meth:`wait_for_job`. - The *job_id* parameter is optional and can be used to specify the ID of the - newly created job. It must be a UUID. - - .. note :: - - The functionality to override the Job ID is not generally available. - :param model_name: Name of the model to train :param dataset_id: Id of previously uploaded, valid dataset :param model_template_id: Model template ID for training - :param job_id: Optionally provide job UUID :raises TrainingJobFailed: When training job has status FAILED :raises TrainingJobTimeOut: When training job takes too long :return: API response as dict @@ -219,7 +197,6 @@ def create_job_and_wait( model_name=model_name, dataset_id=dataset_id, model_template_id=model_template_id, - job_id=job_id, ) return self.wait_for_job(job_resource["id"]) @@ -368,7 +345,7 @@ def read_deployment_by_id(self, deployment_id: str) -> dict: ) return response.json() - def create_deployment(self, model_name: str, deployment_id: str = None) -> dict: + def create_deployment(self, model_name: str) -> dict: """ Creates a Deployment for the given model_name. @@ -379,22 +356,11 @@ def create_deployment(self, model_name: str, deployment_id: str = None) -> dict: :meth:`read_deployment_by_id` or the higher-level :meth:`wait_for_deployment` to poll for status changes. - The *deployment_id* parameter is optional and can be used to specify the ID of - the newly created Deployment. - - .. note :: - - The functionality to override the Deployment ID is not generally available. - :param model_name: name of the Model to deploy - :param deployment_id: Optionally provide deployment identifier :return: a single Deployment as dict """ self.log.info("Creating Deployment for model_name '%s'", model_name) payload = {"modelName": model_name} - if deployment_id: - self.log.info("Deployment ID override specified: %s", deployment_id) - payload["id"] = deployment_id response = self.session.post_to_endpoint( ModelManagerPaths.ENDPOINT_DEPLOYMENT_COLLECTION, payload=payload ) @@ -503,28 +469,20 @@ def polling_function(): return response - def deploy_and_wait(self, model_name: str, deployment_id: str = None) -> dict: + def deploy_and_wait(self, model_name: str) -> dict: """ Deploys a Model and waits for Deployment to succeed. This method is a thin wrapper around :meth:`create_deployment` and :meth:`wait_for_deployment`. - The *deployment_id* parameter is optional and can be used to specify the ID of - the newly created Deployment. - - .. note :: - - The functionality to override the Deployment ID is not generally available. - :param model_name: Name of the Model to deploy - :param deployment_id: Optionally provide deployment identifier :raises DeploymentTimeOut: If Deployment does not finish within timeout :raises DeploymentFailed: If Deployment fails :return: Model resource from final API call """ deployment = self.create_deployment( - model_name=model_name, deployment_id=deployment_id + model_name=model_name, ) deployment_id = deployment["id"] assert deployment_id is not None # for mypy diff --git a/tests/sap/aibus/dar/client/test_model_manager_client.py b/tests/sap/aibus/dar/client/test_model_manager_client.py index 42efd62..f3fef0a 100644 --- a/tests/sap/aibus/dar/client/test_model_manager_client.py +++ b/tests/sap/aibus/dar/client/test_model_manager_client.py @@ -148,34 +148,6 @@ def test_create_job(self, model_manager_client): == response ) - def test_create_job_with_external_job_id(self, model_manager_client): - model_template_id = "d7810207-ca31-4d4d-9b5a-841a644fd81f" - dataset_id = "a2058037-2ae4-465e-8110-65381d47f3d4" - model_name = "my_test_model" - job_id = "24b38880-e304-4d0e-87cf-b9feb8b21393" - - model_manager_client.create_job( - model_template_id=model_template_id, - dataset_id=dataset_id, - model_name=model_name, - job_id=job_id, - ) - - expected_url = "/model-manager/api/v3/jobs" - expected_payload = { - "datasetId": dataset_id, - "modelTemplateId": model_template_id, - "modelName": model_name, - "id": job_id, - } - - expected_post_call = [call(expected_url, payload=expected_payload)] - - assert ( - expected_post_call - == model_manager_client.session.post_to_endpoint.call_args_list - ) - def test_wait_for_job_uses_polling(self, model_manager_client: ModelManagerClient): """ Tests the interaction of the `wait_for_job` method with the Polling class. @@ -299,57 +271,6 @@ def test_create_job_and_wait(self, model_manager_client: ModelManagerClient): model_template_id=model_template_id, dataset_id=dataset_id, model_name=model_name, - job_id=None, - ) - - assert model_manager_client.create_job.call_args_list == [ - expected_create_job_call_args - ] - - expected_wait_for_job_call_args = call(job_resource["id"]) - - assert model_manager_client.wait_for_job.call_args_list == [ - expected_wait_for_job_call_args - ] - - def test_create_job_and_wait_with_external_job_id( - self, model_manager_client: ModelManagerClient - ): - """ - Tests if start_job_and_wait correctly orchestrates start_job() - and wait_for_job() if an external Job ID is used. - """ - model_template_id = "d7810207-ca31-4d4d-9b5a-841a644fd81f" - dataset_id = "a2058037-2ae4-465e-8110-65381d47f3d4" - model_name = "my_test_model" - job_id = "6f122888-3ab9-4485-9b10-d1b982239ab9" - - job_resource = self._make_job_resource("SUCCEEDED") - job_resource["id"] = job_id - - model_manager_client.create_job = create_autospec( - model_manager_client.create_job - ) - model_manager_client.create_job.return_value = job_resource - - model_manager_client.wait_for_job = create_autospec( - model_manager_client.wait_for_job - ) - - ret_val = model_manager_client.create_job_and_wait( - model_name=model_name, - dataset_id=dataset_id, - model_template_id=model_template_id, - job_id=job_id, - ) - - assert ret_val == model_manager_client.wait_for_job.return_value - - expected_create_job_call_args = call( - model_template_id=model_template_id, - dataset_id=dataset_id, - model_name=model_name, - job_id=job_id, ) assert model_manager_client.create_job.call_args_list == [ @@ -546,25 +467,6 @@ def test_create_deployment(self, model_manager_client: ModelManagerClient): == response ) - def test_create_deployment_with_external_id( - self, model_manager_client: ModelManagerClient - ): - model_name = "my_test_model" - deployment_id = "abcd-deployment-id" - - model_manager_client.create_deployment( - model_name=model_name, deployment_id=deployment_id - ) - - expected_url = "/model-manager/api/v3/deployments" - expected_payload = {"modelName": model_name, "id": deployment_id} - expected_post_call = [call(expected_url, payload=expected_payload)] - - assert ( - expected_post_call - == model_manager_client.session.post_to_endpoint.call_args_list - ) - def _make_deployment_resource(self, status): return make_deployment_resource(status) @@ -720,55 +622,7 @@ def test_deploy_and_wait(self, model_manager_client: ModelManagerClient): # assert assert return_value == model_manager_client.wait_for_deployment.return_value - expected_call_to_create_deployment = call( - model_name=model_name, deployment_id=None - ) - - assert model_manager_client.create_deployment.call_args_list == [ - expected_call_to_create_deployment - ] - - expected_call_to_wait_for_deployment = call( - deployment_id=deployment_resource["id"] - ) - - assert model_manager_client.wait_for_deployment.call_args_list == [ - expected_call_to_wait_for_deployment - ] - - def test_deploy_and_wait_with_external_deployment_id( - self, model_manager_client: ModelManagerClient - ): - """ - Tests if *deploy_and_wait* orchestrates *create_deployment* and - *wait_for_deployment* correctly if an external deployment ID is given. - """ - - deployment_id = "abcd" - model_name = "my-test-model" - - # prepare - deployment_resource = self._make_deployment_resource("PENDING") - deployment_resource["id"] = deployment_id - model_manager_client.create_deployment = create_autospec( - model_manager_client.create_deployment, return_value=deployment_resource - ) - - model_manager_client.wait_for_deployment = create_autospec( - model_manager_client.wait_for_deployment - ) - - # act - return_value = model_manager_client.deploy_and_wait( - model_name=model_name, deployment_id=deployment_id - ) - - # assert - assert return_value == model_manager_client.wait_for_deployment.return_value - - expected_call_to_create_deployment = call( - model_name=model_name, deployment_id=deployment_id - ) + expected_call_to_create_deployment = call(model_name=model_name) assert model_manager_client.create_deployment.call_args_list == [ expected_call_to_create_deployment From c1db65fb605ac7b819a026ae0ed10c3944695874 Mon Sep 17 00:00:00 2001 From: Michael Haas Date: Fri, 17 Sep 2021 09:27:09 +0200 Subject: [PATCH 2/3] Update CHANGELOG.md --- CHANGELOG.md | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ee242ad..4d1a6ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.8.2] + +### Changed + +* Remove passing of external deployment ID and job ID from SDK API. This functionality + never materialized in the Data Attribute Recommendation service and is now also removed in + the SDK. + This is an API change in the SDK because it changes the method signature of some + methods to remove the optional `job_id` and `deployment_id` arguments. Passing these arguments + always resulted in an error returned by the Data Attribute Recommendation service. For this + reason, this change is not a breaking change. + +[#107]: https://github.com/SAP/data-attribute-recommendation-python-sdk/pull/107 + ## [0.8.1] ### Fixed @@ -176,7 +190,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * First public release -[Unreleased]: https://github.com/SAP/data-attribute-recommendation-python-sdk/compare/rel/0.8.1...HEAD +[Unreleased]: https://github.com/SAP/data-attribute-recommendation-python-sdk/compare/rel/0.8.2...HEAD +[0.8.2]: https://github.com/SAP/data-attribute-recommendation-python-sdk/compare/rel/0.8.1...rel/0.8.2 [0.8.1]: https://github.com/SAP/data-attribute-recommendation-python-sdk/compare/rel/0.8.0...rel/0.8.1 [0.8.0]: https://github.com/SAP/data-attribute-recommendation-python-sdk/compare/rel/0.7.1...rel/0.8.0 [0.7.1]: https://github.com/SAP/data-attribute-recommendation-python-sdk/compare/rel/0.7.0...rel/0.7.1 @@ -201,6 +216,3 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#8]: https://github.com/SAP/data-attribute-recommendation-python-sdk/pull/8 [#16]: https://github.com/SAP/data-attribute-recommendation-python-sdk/pull/16 [0.6.2]: https://github.com/SAP/data-attribute-recommendation-python-sdk/tree/rel/0.6.2 - - - From 4b13d02bede309344d711410010e5f7eaa547d96 Mon Sep 17 00:00:00 2001 From: Michael Haas Date: Fri, 17 Sep 2021 09:31:09 +0200 Subject: [PATCH 3/3] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d1a6ff..8ef0fd0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 methods to remove the optional `job_id` and `deployment_id` arguments. Passing these arguments always resulted in an error returned by the Data Attribute Recommendation service. For this reason, this change is not a breaking change. + This change effectively reverts [#98]. [#107]: https://github.com/SAP/data-attribute-recommendation-python-sdk/pull/107