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
test sframe s3 download and upload #3054
test sframe s3 download and upload #3054
Conversation
else: | ||
self.assertEqual(v1, v2) | ||
|
||
def _test_sframe_equal(self, sf1, sf2): |
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 you just use the method we already have for doing this?
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.
Yeah, why not. I hope I know it earlier.
region_name=os.environ["TURI_S3_REGION"], | ||
) | ||
self.bucket = "tc_qa" | ||
self.s3_prefix = "integration/manual/" |
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.
I'm not sure "manual" makes sense in the prefix.
Much more importantly - if two different instances of these tests are running at the same time, they will step on each other. I suggest you generate a UUID and append that to the prefix.
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.
that's my s3 prefix, not the prefix on local fs.
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.
and the file is stored in S3, without uuid.
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.
Right, it looks like it always writes to the same place in S3. So if two instances of this script are running at the same time they will be interfering with each other because they're both writing and reading to the same place.
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.
I believe s3 will attach version id to it. And our internal S3 won't allow us to delete folders.
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.
that's why I'm not going to have a UUID as a grouping folder.
depends on boto/boto3#2347 |
add test for frame s3 download and upload.
define env var
DTURI_ENDBALE_SF_S3
to enable the test.add listed env vars in CI,
to run the test.