-
Notifications
You must be signed in to change notification settings - Fork 367
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.
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
[Feature] Add OSSBackend for reading data from aliyun oss #834
base: main
Are you sure you want to change the base?
Conversation
Hi @liuxianyi , thanks for your contribution. Added |
ok, but I don't how can I test those units without init.py.
so, can you give a solution? |
What command did you run to test unit tests? |
|
You can directly run |
Codecov ReportBase: 78.66% // Head: 78.88% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #834 +/- ##
==========================================
+ Coverage 78.66% 78.88% +0.21%
==========================================
Files 128 129 +1
Lines 9348 9485 +137
Branches 1848 1877 +29
==========================================
+ Hits 7354 7482 +128
- Misses 1679 1684 +5
- Partials 315 319 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
i get the same error:
|
Have you installed |
|
Yes, you need to install mmengine with |
ok, thank you |
You can update the branch with the latest main. |
access_key_id: Optional[str] = None, | ||
access_key_secret: Optional[str] = None, | ||
path_mapping: Optional[dict] = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be convenient if an endpoint is passed as an initialization parameter. Users can use the interface without an endpoint.
from mmengine.fileio import OSSBackend
backend = OSSBackend(
access_key_id=xxx',
access_key_secret='xxx',
endpoint='oss-cn-hangzhou.aliyuncs.com')
# this can cover most user cases
backend.put_text('hello world!', 'oss://mmengine-test/text1.txt')
# also support the URL containing the endpoint
# users maybe want to use another endpoint so users can pass an endpoint
backend.put_text('hello world!', 'oss://oss-cn-hanzhou.aliyuncs.com/mmengine-test/text1.txt')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,but it may occur that the Endpoint conflicts with folder names, resulting in regular match failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can an access_key_id
correspond to multiple endpoints? If not, the file path can not contain an endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an access_key_id
may be for multiple endpoints, so I think that the endpoint needs to be considered.
root_path = obj_name | ||
if root_path and not root_path.endswith('/'): | ||
raise TypeError('`dir_path` must endswith "/" ') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After implementing the is_dir
method, there is no need to require root_path
ending with /
.
f'does not equals src file size {total_size}, please recopy.' | ||
return str(dst) | ||
|
||
def list_dir_or_file(self, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this method also support oss://endpoint/bucket/
?
cls.oss_dir = 'oss://oss-cn-hangzhou.aliyuncs.com/mmengine/' | ||
local_oss_ak_path = cls.test_data_dir / 'AccessKey.csv' | ||
access_key = pd.read_csv(local_oss_ak_path, sep=',') | ||
cls.access_key_id = access_key.loc[0, 'AccessKey ID'] | ||
cls.access_key_secret = access_key.loc[0, 'AccessKey Secret'] | ||
print(f'access_key_id: {cls.access_key_id},\ | ||
access_key_secret: {cls.access_key_secret}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be convenient to read them from environments and then other developers can also test this unit test.
cls.oss_dir = os.getenv('MMEngine_OSS_TEST_DIR')
cls.access_key_id = os.getenv('MMENGINE_OSS_ACCESS_KEY_ID')
cls.access_key_secret = os.getenv('MMENGINE_OSS_ACCESS_KEY_SECRET')
Hi @liuxianyi !We are grateful for your efforts in helping improve this open-source project during your personal time. Welcome to join OpenMMLab Special Interest Group (SIG) private channel on Discord, where you can share your experiences, ideas, and build connections with like-minded peers. To join the SIG channel, simply message moderator— OpenMMLab on Discord or briefly share your open-source contributions in the #introductions channel and we will assist you. Look forward to seeing you there! Join us :https://discord.gg/UjgXkPWNqA Thank you again for your contribution❤ |
Thanks for your contribution and we appreciate it a lot. The following instructions would make your pull request more healthy and more easily get feedback. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.
Motivation
currently, mmengine can't support the OSS of Aliyun, so I pull a request to add this function.
Modification
tests/test_fileio
, specifically,mmengine/fileio/backends/OSS_backend.py
for function-related read and write.mmengine/fileio/file_client.py
add into '_prefix_to_backends'.tests/test_fileio/test_backends/test_OSS_backend.py
for ensuring the function formally operate.BC-breaking (Optional)
Does the modification introduce changes that break the backward-compatibility of the downstream repos?
If so, please describe how it breaks the compatibility and how the downstream projects should modify their code to keep compatibility with this PR.
Use cases (Optional)
If this PR introduces a new feature, it is better to list some use cases here, and update the documentation.
Checklist