Skip to content

Revert: pass external job_id and deployment_id #107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 22, 2021
Merged

Revert: pass external job_id and deployment_id #107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eaac564
Revert: pass external job_id and deployment_id
mhaas Sep 17, 2021
c1db65f
Update CHANGELOG.md
mhaas Sep 17, 2021
4b13d02
Update CHANGELOG
mhaas Sep 17, 2021
89b713b
Merge branch 'main' into revert_external_deployment
karthikmswamy Sep 20, 2021
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
21 changes: 17 additions & 4 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,21 @@ 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.
This change effectively reverts [#98].

[#107]: https://github.com/SAP/data-attribute-recommendation-python-sdk/pull/107

## [0.8.1]

### Fixed
Expand Down Expand Up @@ -176,7 +191,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
Expand All @@ -201,6 +217,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



48 changes: 3 additions & 45 deletions sap/aibus/dar/client/model_manager_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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.
Expand All @@ -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(
Expand All @@ -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
)
Expand All @@ -192,25 +179,16 @@ 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.

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
Expand All @@ -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"])

Expand Down Expand Up @@ -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.

Expand All @@ -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
)
Expand Down Expand Up @@ -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
Expand Down
148 changes: 1 addition & 147 deletions tests/sap/aibus/dar/client/test_model_manager_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 == [
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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
Expand Down