Add 'set_task_metadata', associated test, and example in README#56
Conversation
| endpoint = f"task/{task_id}/unique_id" | ||
| return Task(self.api.delete_request(endpoint), self) | ||
|
|
||
| def set_task_metadata(self, task_id: str, metadata: json) -> Task: |
There was a problem hiding this comment.
Just thinking about whether we should expect a JSON or a Dict/object as input?
There was a problem hiding this comment.
Revised to expect Dict instead of JSON
| .. code-block :: python | ||
|
|
||
| # set metadata on a task to be the contents of new_metadata | ||
| new_metadata = {'metadata': {'myKey': 'myValue'}} |
There was a problem hiding this comment.
I think we append metadata in the method? So users should only pass {'myKey': 'myValue'}?
There was a problem hiding this comment.
Also should we govern that in the method? When they check the API guide they may think they need to pass metadata as well, but appending that ourselves is probably a better user experience.
There was a problem hiding this comment.
Possible oversight here, but the current implementation of the API endpoint does expect that the customer will provide the metadata in the format of `{"metadata": {"key": "value"}}.
Should we revise the endpoint and also update the SDK to reflect this change?
There was a problem hiding this comment.
Ah.. yeah, we probably should - sorry I missed that nuance.
There was a problem hiding this comment.
No matter what we choose, I think we should be consistent all over the place.
If endpoint expects a metadata as the main key then we should:
- expect user to follow that format
- is this enforced in API? If not, we may also govern that in SDK as an additional measure. metadata key to be there, be an object? and be the only key in the main object?
- We may not add extra metadata in the method: https://github.com/scaleapi/scaleapi-python-client/pull/56/files#diff-09863685c9b788d64d266a4468f8e7f893e1d1c7773c9e20c6757b70fd378608R130 just expect user to do that if API expects.
payload = {"metadata": metadata}
Or we should update the API just to accept any object and store under metadata. (so users does not need to add extra metadata field) -> I'm pro to this.
There was a problem hiding this comment.
The API currently does enforce having the "metadata" key
I'll revise the logic in the API and then revise the behavior in the SDK today so that they are consistent
There was a problem hiding this comment.
PR created for change to API endpoint: https://github.com/scaleapi/scaleapi/pull/47725
|
The set metadata test will fail until the change to the API endpoint is merged and deployed |
|
@fatihkurtoglu Within this branch I've bumped the version to Could you help me review/approve when able? |
fatihkurtoglu
left a comment
fatihkurtoglu
left a comment
There was a problem hiding this comment.
@kevin-xu-scale looks good to me. I would also implement set_metadata as a method under Task object too. i.e. we have cancel_task at API level, but also implemented Task.cancel().
scaleapi-python-client/scaleapi/tasks.py
Lines 87 to 89 in 4c46e1a
| endpoint = f"task/{task_id}/unique_id" | ||
| return Task(self.api.delete_request(endpoint), self) | ||
|
|
||
| def set_task_metadata(self, task_id: str, metadata: dict) -> Task: |
There was a problem hiding this comment.
| def set_task_metadata(self, task_id: str, metadata: dict) -> Task: | |
| def set_task_metadata(self, task_id: str, metadata: Dict) -> Task: |
| Set A Task's Metadata | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| Set a given task's `metadata`. Check out `Scale's API documentation`__ for more information. |
There was a problem hiding this comment.
I think in .rst files, double tick makes it as a code format.
| Set a given task's `metadata`. Check out `Scale's API documentation`__ for more information. | |
| Set a given task's ``metadata``. Check out `Scale's API documentation`__ for more information. |
Once this PR is merged, it will have the 2.10.0 version in the master. Then you can release your version with the tag |
|
@fatihkurtoglu changes implemented, new test added, squashing and merging now |
3 participants
No description provided.