Skip to content
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

Add backend to support uploading large files. #2906

Merged
merged 36 commits into from
Aug 16, 2020

Conversation

KhalidRmb
Copy link
Member

Adds APIs and util methods to enable generation of a presigned url and enable the pipeline to upload large annotation files and submission files through CLI.

Related: Cloud-CV/evalai-cli#277

apps/base/utils.py Outdated Show resolved Hide resolved
apps/challenges/views.py Outdated Show resolved Hide resolved
apps/base/utils.py Outdated Show resolved Hide resolved
apps/challenges/views.py Outdated Show resolved Hide resolved
apps/jobs/views.py Outdated Show resolved Hide resolved
@KhalidRmb KhalidRmb changed the title (WIP) Add backend to support uploading large files. Add backend to support uploading large files. Jul 16, 2020
@KhalidRmb
Copy link
Member Author

@RishabhJain2018 @deshraj @Ram81 PTAL.

apps/base/utils.py Outdated Show resolved Hide resolved
apps/base/utils.py Outdated Show resolved Hide resolved
apps/challenges/views.py Outdated Show resolved Hide resolved
apps/challenges/views.py Show resolved Hide resolved
apps/jobs/views.py Outdated Show resolved Hide resolved
apps/jobs/views.py Outdated Show resolved Hide resolved
apps/jobs/views.py Outdated Show resolved Hide resolved
apps/challenges/views.py Show resolved Hide resolved
apps/base/utils.py Outdated Show resolved Hide resolved
apps/jobs/views.py Show resolved Hide resolved
apps/jobs/urls.py Outdated Show resolved Hide resolved
apps/jobs/views.py Outdated Show resolved Hide resolved
apps/jobs/views.py Outdated Show resolved Hide resolved
apps/jobs/views.py Outdated Show resolved Hide resolved
apps/jobs/views.py Outdated Show resolved Hide resolved
Copy link
Member

@Ram81 Ram81 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 . @RishabhJain2018 can we delpoy this to staging. I can test it once on staging

@@ -138,6 +138,17 @@ def send_email(
return


def get_aws_secret_keys():
Copy link
Member

Choose a reason for hiding this comment

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

We do have this function, can you please use that instead of creating a new one.
cc: @Ram81

Copy link
Member Author

Choose a reason for hiding this comment

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

Ca you point me to this? Didn't find one in base/utils.py

Copy link
Member

Choose a reason for hiding this comment

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

I think it is in challenge/utils.py

Copy link
Member Author

@KhalidRmb KhalidRmb Jul 26, 2020

Choose a reason for hiding this comment

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

Okay...I've changed the function to get the storage bucket name as well when we are not using host's credentials.
But, do we need to add the bucket name field to the challenge model as well, to get the host's bucket name to the aws_keys when using host credentials? (I can make a separate PR for this.) @RishabhJain2018 @Ram81

Original function for reference:

def get_aws_credentials_for_challenge(challenge_pk):

apps/base/utils.py Outdated Show resolved Hide resolved
apps/challenges/urls.py Outdated Show resolved Hide resolved
apps/base/utils.py Outdated Show resolved Hide resolved
@@ -168,6 +179,31 @@ def get_boto3_client(resource, aws_keys):
logger.exception(e)


def get_presigned_url_for_file_upload(file_key):
"""
Function to get the presigned url to upload a file to s3 with name file_name and key as file_key.
Copy link
Member

Choose a reason for hiding this comment

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

file_key -> Can we please change it something more meaningful.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's clear that it's the key for the file on S3 right. What else can we call it... file_key_on_s3? cc: @Ram81

apps/jobs/views.py Outdated Show resolved Hide resolved
apps/jobs/views.py Outdated Show resolved Hide resolved
apps/jobs/urls.py Outdated Show resolved Hide resolved
apps/jobs/views.py Outdated Show resolved Hide resolved
apps/jobs/urls.py Outdated Show resolved Hide resolved
apps/base/utils.py Outdated Show resolved Hide resolved
@RishabhJain2018
Copy link
Member

@KhalidRmb Is this complete?
cc: @Ram81

@KhalidRmb
Copy link
Member Author

@KhalidRmb Is this complete?
cc: @Ram81

@RishabhJain2018 Yes

@Ram81
Copy link
Member

Ram81 commented Aug 3, 2020

@RishabhJain2018 I haven't tested the PR yet. Are we planning to test this one staging?

@RishabhJain2018
Copy link
Member

@RishabhJain2018 I haven't tested the PR yet. Are we planning to test this one staging?

Yes!

@Ram81
Copy link
Member

Ram81 commented Aug 4, 2020

@KhalidRmb did you also test these changes (for test annotations) with auto launch workers feature when challenge is approved? Because we will be auto approving challenges and launching workers before challenge approval would this break submission worker launch flow?
cc: @RishabhJain2018

@KhalidRmb
Copy link
Member Author

KhalidRmb commented Aug 4, 2020

@KhalidRmb did you also test these changes (for test annotations) with auto launch workers feature when challenge is approved? Because we will be auto approving challenges and launching workers before challenge approval would this break submission worker launch flow?

I thought the admin won't approve a challenge without an annotation file. But if we're auto approving a challenge, the worker will fail after starting since there's no file yet to fetch. We'll have to catch this case in the auto approval pipeline, where we should check for annotation file being present before auto approving a challenge?

On top this I think we should show an appropriate error message is case the host tries to start the worker through manage tab without an annotation file.

@Ram81
Copy link
Member

Ram81 commented Aug 14, 2020

@KhalidRmb can you resolve conflicts so that we can test this on staging

@codecov-commenter
Copy link

Codecov Report

Merging #2906 into master will increase coverage by 1.09%.
The diff coverage is 25.99%.

@@            Coverage Diff             @@
##           master    #2906      +/-   ##
==========================================
+ Coverage   72.93%   74.03%   +1.09%     
==========================================
  Files          83       20      -63     
  Lines        5368     3019    -2349     
==========================================
- Hits         3915     2235    -1680     
+ Misses       1453      784     -669     
Impacted Files Coverage Δ
...ontend/src/js/controllers/featuredChallengeCtrl.js 68.99% <ø> (ø)
frontend/src/js/controllers/authCtrl.js 58.58% <3.70%> (-8.28%) ⬇️
frontend/src/js/controllers/permissionCtrl.js 36.36% <22.22%> (-63.64%) ⬇️
frontend/src/js/controllers/challengeCtrl.js 66.18% <22.48%> (-7.52%) ⬇️
frontend/src/js/controllers/challengeListCtrl.js 95.74% <50.00%> (+1.06%) ⬆️
...ntend/src/js/controllers/challengeHostTeamsCtrl.js 70.50% <66.66%> (-1.18%) ⬇️
frontend/src/js/controllers/teamsCtrl.js 71.17% <66.66%> (ø)
frontend/src/js/controllers/changePwdCtrl.js 100.00% <100.00%> (ø)
frontend/src/js/controllers/dashCtrl.js 99.00% <100.00%> (+0.08%) ⬆️
settings/common.py
... and 36 more
Impacted Files Coverage Δ
...ontend/src/js/controllers/featuredChallengeCtrl.js 68.99% <ø> (ø)
frontend/src/js/controllers/authCtrl.js 58.58% <3.70%> (-8.28%) ⬇️
frontend/src/js/controllers/permissionCtrl.js 36.36% <22.22%> (-63.64%) ⬇️
frontend/src/js/controllers/challengeCtrl.js 66.18% <22.48%> (-7.52%) ⬇️
frontend/src/js/controllers/challengeListCtrl.js 95.74% <50.00%> (+1.06%) ⬆️
...ntend/src/js/controllers/challengeHostTeamsCtrl.js 70.50% <66.66%> (-1.18%) ⬇️
frontend/src/js/controllers/teamsCtrl.js 71.17% <66.66%> (ø)
frontend/src/js/controllers/changePwdCtrl.js 100.00% <100.00%> (ø)
frontend/src/js/controllers/dashCtrl.js 99.00% <100.00%> (+0.08%) ⬆️
settings/common.py
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 334de84...707be4d. Read the comment docs.

@RishabhJain2018 RishabhJain2018 merged commit ac416ec into Cloud-CV:master Aug 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants