[batch] use volcano TOS as batch storage #344
Conversation
python/aibrix/tests/test_storage.py
Outdated
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| test_submit_job_input() |
There was a problem hiding this comment.
why not to use pytest here? and seems you only invoke one test method?
There was a problem hiding this comment.
I remove the main. pytest is enabled here by default.
There was a problem hiding this comment.
BTW, please move it under batch folder etc if this is specific to batch feature. it can be done in future PRs
| ) | ||
|
|
||
|
|
||
| def initialize_storage(storage_type=StorageType.LocalDiskFile, params={}): |
There was a problem hiding this comment.
nit: what's the different between initialize_storage and initialize_batch_storage? why do we need two levels?
There was a problem hiding this comment.
this is to be consistent with other interfaces.
| @@ -14,14 +14,30 @@ | |||
|
|
|||
| import uuid | |||
|
|
|||
| from aibrix.batch.storage.tos_storage import TOSStorage | |||
There was a problem hiding this comment.
please also add S3 later. You can use minio for testing
|
|
||
| self._client = None | ||
| self._bucket_name = bucket_name | ||
| endpoint = "tos-cn-beijing.volces.com" |
There was a problem hiding this comment.
can we ask user to pass in the endpoint and region. they should be same as credentials. We can not fix it here. We already have cluster setup for other regions
| ) | ||
|
|
||
| print("Finished creating TOS client!!!") | ||
| # HTTP状态码 |
There was a problem hiding this comment.
can you remove chinese characters?
| self._bucket_name, object_key, content=string_io_obj | ||
| ) | ||
|
|
||
| print("Finished creating TOS client!!!") |
There was a problem hiding this comment.
can you use logging instead of print?
There was a problem hiding this comment.
why do you need to create object as part of client initialization
| try: | ||
| self._client = tos.TosClientV2(ak, sk, endpoint, region) | ||
| object_key = "test_key" | ||
| string_io_obj = StringIO("hello world") |
There was a problem hiding this comment.
what are you doing here..?
| ) | ||
| except tos.exceptions.TosServerError as e: | ||
| # 操作失败，捕获服务端异常，可从返回信息中获取详细错误信息 | ||
| print("fail with server error, code: {}".format(e.code)) |
There was a problem hiding this comment.
why not create a single message and custom the error string?
There was a problem hiding this comment.
Seems this is not done yet. this is minor issue. please help refactor it in later PRs.
except tos.exceptions.TosServerError as e:
logging.error(f"fail with server error, code: {e.code}")
logging.error(f"error with request id: {e.request_id}")
logging.error(f"error with message: {e.message}")
logging.error(f"error with http code: {e.status_code}")
logging.error(f"error with ec: {e.ec}".format())
logging.error(f"error with request url: {e.request_url}")
|
All comments have been addressed. This PR I only change the print to logging in TOS file. Next PR I will change all print to logging. @Jeffwan |
|
It looks good to me. please address unfinished items in future PR. I highly suggest to concentrate on e2e testing in future PRs and list down the blocker items so we can better build the plans |
* use TOS as batch request storage * update format check * file with test * update tos with volcano TOS and add an initialization for storage * update format * address comments * remove main * update log format * update log format --------- Co-authored-by: xin.chen <xin.chen@bytedance.com>
Pull Request Description
This PR uses remote TOS to save all Batch requests, including input and output.
Now this PR uses volcano TOS.
Related Issues
Resolves: part of #182
Contribution Guidelines (Expand for Details)
We appreciate your contribution to aibrix! To ensure a smooth review process and maintain high code quality, please adhere to the following guidelines:
Pull Request Title Format
Your PR title should start with one of these prefixes to indicate the nature of the change:
[Bug]: Corrections to existing functionality[CI]: Changes to build process or CI pipeline[Docs]: Updates or additions to documentation[API]: Modifications to aibrix's API or interface[CLI]: Changes or additions to the Command Line Interface[Misc]: For changes not covered above (use sparingly)Note: For changes spanning multiple categories, use multiple prefixes in order of importance.
Submission Checklist
By submitting this PR, you confirm that you've read these guidelines and your changes align with the project's contribution standards.