Skip to content

Conversation

@nanaya-tachibana
Copy link
Contributor

@nanaya-tachibana nanaya-tachibana commented May 2, 2019

#1954

  • Have you followed the guidelines in our
    Contributing document?

  • Does your PR affect documented changes or does it add new functionality
    that should be documented? If yes, have you created a PR for
    dvc.org documenting it or at
    least opened an issue for it? If so, please add a link to it. link


@efiop
Copy link
Contributor

efiop commented May 2, 2019

Hi @nanaya-tachibana !

Thanks for submitting your PR! Looks pretty good! 🙂

  1. Please see that there are some failing tests;
  2. Is there something we could use to test oss without an oss account? Something like azurite for azure maybe? And maybe there is something we could use for unit tests too?

Thanks,
Ruslan

nanaya-tachibana added a commit to nanaya-tachibana/dvc that referenced this pull request May 4, 2019
@nanaya-tachibana nanaya-tachibana force-pushed the master branch 2 times, most recently from 098be92 to a8031ca Compare May 4, 2019 08:51
@nanaya-tachibana
Copy link
Contributor Author

Hi @nanaya-tachibana !

Thanks for submitting your PR! Looks pretty good! 🙂

  1. Please see that there are some failing tests;
  2. Is there something we could use to test oss without an oss account? Something like azurite for azure maybe? And maybe there is something we could use for unit tests too?

Thanks,
Ruslan

I found an old oss emulate and made it compatible with the new oss python SDK.
Now we can test oss by running an emulate in docker.

Start a container running an oss emulator.
$ git clone https://github.com/nanaya-tachibana/oss-emulator.git
$ docker image build -t oss:1.0 oss-emulator
$ docker run --detach -p 8880:8880 --name oss-emulator oss:1.0
Setup environment variables.
$ export OSS_BUCKET='my-bucket'
$ export OSS_ENDPOINT='localhost:8880'
$ export OSS_ACCESS_KEY_ID='AccessKeyID'
$ export OSS_ACCESS_KEY_SECRET='AccessKeySecret'

I also added these processes to travis and appveyor setting files. It was my first time using these continuous integration services. I hope I did correctly.

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Please see a few more comments down below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to remove cli test? I commented this test because the gc cli test failed after I removed the codes in cache.py.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GC cli test? Google Cloud? Or garbage collector?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what was the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Traceback (most recent call last):
File "/Users/nanaya/Projects/dvc/dvc/main.py", line 38, in main
ret = cmd.run_cmd()
File "/Users/nanaya/Projects/dvc/dvc/command/base.py", line 60, in run_cmd
return self.run()
File "/Users/nanaya/Projects/dvc/dvc/command/gc.py", line 47, in run
repos=self.args.repos,
File "/Users/nanaya/Projects/dvc/dvc/repo/gc.py", line 113, in gc
_do_gc("remote", self.cloud._get_cloud(remote, "gc -c").gc, clist)
File "/Users/nanaya/Projects/dvc/dvc/repo/gc.py", line 54, in _do_gc
removed = func(clist)
File "/Users/nanaya/Projects/dvc/dvc/remote/base.py", line 489, in gc
used |= {info[self.PARAM_CHECKSUM] for info in cinfos[self.scheme]}
KeyError: 'oss'

cinfos is a dict which only has keys for 'local', 's3', 'gs', 'hdfs', 'ssh' and 'azure'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nanaya-tachibana Thanks! Looks like a bug. Does replacing cinfos[self.scheme] with cinfos.get(self.scheme, []) help?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nanaya-tachibana Thanks! Looks like a bug. Does replacing cinfos[self.scheme] with cinfos.get(self.scheme, []) help?

That can fix it. Should I add another commit to fix it and keep the command line tests?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nanaya-tachibana If you would be so kind 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, decided to help out a little bit 🙂

@efiop
Copy link
Contributor

efiop commented May 7, 2019

@nanaya-tachibana Btw, please rebase your patch on top of master.

@efiop
Copy link
Contributor

efiop commented May 7, 2019

@nanaya-tachibana looks like you didn't rebase correctly. Try something like:

git remote add upstream https://github.com/iterative/dvc
git fetch upstream
git rebase upstream/master
git push -f

Usage:
$ dvc remote add myremote oss://my-bucket.endpoint/path
Set key id and key secret using modify command
$ dvc remote modify myremote oss_key_id my-key-id
$ dvc remote modify myremote oss_key_secret my-key-secret
or environment variables
$ export OSS_ACCESS_KEY_ID="my-key-id"
$ export OSS_ACCESS_KEY_SECRET="my-key-secret"

Ref:
oss python SDK: https://www.alibabacloud.com/help/doc-detail/32026.htm
Start a container running an oss emulator.
$ git clone https://github.com/nanaya-tachibana/oss-emulator.git
$ docker image build -t oss:1.0 oss-emulator
$ docker run --detach -p 8880:8880 --name oss-emulator oss:1.0
Setup environment variables.
$ export OSS_BUCKET='my-bucket'
$ export OSS_ENDPOINT='localhost:8880'
$ export OSS_ACCESS_KEY_ID='AccessKeyID'
$ export OSS_ACCESS_KEY_SECRET='AccessKeySecret'
which gives read access to public read bucket and public bucket.
…able value.

Usage:
$ dvc remote add myremote oss://my-bucket/path
Set key id, key secret and endpoint using modify command
$ dvc remote modify myremote oss_key_id my-key-id
$ dvc remote modify myremote oss_key_secret my-key-secret
$ dvc remote modify myremote oss_endpoint endpoint
or environment variables
$ export OSS_ACCESS_KEY_ID="my-key-id"
$ export OSS_ACCESS_KEY_SECRET="my-key-secret"
$ export OSS_ENDPOINT="endpoint"
Ruslan Kuprieiev added 3 commits May 9, 2019 15:02
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Just to keep things in-house.

Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
@efiop
Copy link
Contributor

efiop commented May 9, 2019

@nanaya-tachibana Truly outstanding work! 🥇 It is a pleasure working with you! Thank you so much! 🙂

@efiop efiop merged commit 7ceaf88 into treeverse:master May 9, 2019

class RemoteOSS(RemoteBase):
"""
oss2 document:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

@efiop
Copy link
Contributor

efiop commented May 10, 2019

Hi @nanaya-tachibana !

Thanks again for your contribution! We would love to have a chance to meet you :) Would you be willing to have a hangouts call with us some time? If so, please ping me through email(in my profile) or on discord https://dvc.org/chat (I'm 'ruslan' there), so we could arrange something 🙂

Thanks,
Ruslan

@dragonly
Copy link

there's no docs about adding an oss remote, could you please add the corresponding instructions in the docs? thx

@efiop
Copy link
Contributor

efiop commented Jun 24, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants