[REVIEW PENDING] Add Challenge creation using zip file. #1037

RishabhJain2018 · 2017-06-12T11:34:47Z

fixes #1038 @deshraj @taranjeet Please review the PR.

taranjeet · 2017-06-20T18:36:29Z

@RishabhJain2018 : just a minor suggestion here, Can you please mark WIP or COMPLETE in your PR title so that the one reviewing can easily get an overview. Also it will be great if you keep managing the state (WIP, COMPLETE, ... )after some one has reviewed. You can even use REVIEW PENDING as a state also. Thanks :)

RishabhJain2018 · 2017-06-20T18:56:57Z

@taranjeet yeah sure. I will do that. 👍

taranjeet · 2017-06-21T18:36:39Z

apps/challenges/urls.py

@@ -25,4 +25,7 @@
        name='get_challenges_based_on_teams'),
    url(r'(?P<challenge_pk>[0-9]+)/challenge_phase_split$', views.challenge_phase_split_list,
        name='challenge_phase_split_list'),
+    url(r'challenge/challenge_host_team/(?P<challenge_host_team_pk>[0-9]+)/zip/$',


I am actually not clear of whether we should take challenge host team as a paramter in the api or not. Writing it down here, so that I dont forget. Will update you over this.

Also Can we change the api-endpoint zip to something more apt?

@taranjeet I think we we will need challenge host team as a parameter because it is related as a Foreign Key to Challenge model. What do you say?
Moreover, I will change the endpoint zip to more verbose name. 👍

taranjeet · 2017-06-21T18:39:19Z

apps/challenges/views.py

+                          DatasetSplitSerializer,
+                          LeaderboardSerializer,
+                          ZipConfigurationChallengeSerializer,
+                          ZipFileCreateChallengePhaseSplitSerializer,)


ZipFileCreateChallengePhaseSplitSerializer is too long. Can we please shortened it and still keep it as meaningful?

taranjeet · 2017-06-21T18:41:53Z

apps/challenges/views.py

+        return Response(response_data, status=status.HTTP_400_BAD_REQUEST)
+
+    try:
+        with transaction.atomic():


We use transaction.atomic whenever we want to execute a set of more than db queries as a whole. Explaining this further, it means that we want either the whole set of query should be executed, or None if any of them fails.

Here we have such a use case, but i am really confused of using transaction.atomic to handle code related to http requests(like get request for zip file).

Can you please explain me your reason of using transaction.atomic in such a way?

I have used transaction.atomic because if anything fails then it will go directly into except block ans we don't have to handle cases manually. Moreover, we can have the code in minimal lines. Am I correct, please guide ?

taranjeet · 2017-06-21T18:43:24Z

apps/challenges/views.py

+            # All files download and extract location.
+            BASE_LOCATION = tempfile.mkdtemp()
+
+            CHALLENGE_ZIP_DOWNLOAD_LOCATION = os.path.join('{}/zip_challenge.zip'.format(BASE_LOCATION))


Instead of hardcoding the name of the zip file, can we please make it a random file name. This will help us in dealing with situations where in we get more than one request at the same timestamp. I will prefer the random name to be of 8-10 characters.

ohh..I didn't thought about this case. I will update that accordingly.

taranjeet · 2017-06-21T18:43:55Z

apps/challenges/views.py

+            CHALLENGE_ZIP_DOWNLOAD_LOCATION = os.path.join('{}/zip_challenge.zip'.format(BASE_LOCATION))
+
+            if response and response.status_code == 200:
+                with open(str(CHALLENGE_ZIP_DOWNLOAD_LOCATION), 'w') as f:


CHALLENGE_ZIP_DOWNLOAD_LOCATION is already a string. I think there is no need of using it :)

Also can we make variable f as zip_file or something related.

taranjeet · 2017-06-21T18:47:34Z

apps/challenges/views.py

+                # Search for yaml file
+                for name in zip_ref.namelist():
+                    if name.endswith('.yaml') or name.endswith('.yml'):
+                        yaml_file = name


variable yaml_file is defined only if there is a file ending with extension yaml or yml. What if there is no such file. In that case the code should not proceed further.

RishabhJain2018 · 2017-06-24T23:03:19Z

@deshraj @taranjeet Please review the PR. Once this is approved, I will add the test cases regarding the same.

taranjeet · 2017-06-25T09:46:25Z

apps/challenges/urls.py

@@ -25,4 +25,7 @@
        name='get_challenges_based_on_teams'),
    url(r'(?P<challenge_pk>[0-9]+)/challenge_phase_split$', views.challenge_phase_split_list,
        name='challenge_phase_split_list'),
+    url(r'challenge/challenge_host_team/(?P<challenge_host_team_pk>[0-9]+)/create-challenge-using-zip/$',


The url pattern overall is correct. But we are using http verbs to communicate things like create, delete, edit etc. So the part create-challenge-using-zip is a bit redundant. Can we keep it something like to zip-upload. I am open to alternatives from your side.

taranjeet · 2017-06-25T09:48:46Z

apps/challenges/views.py

+        response = requests.get(uploaded_zip_file_path, stream=True)
+
+        # All files download and extract location.
+        BASE_LOCATION = tempfile.mkdtemp()


We can move this variable outside try-except. Generally we only use part of code which is prone to through error like a network call. This is kind of abusing try-except block. Also it is not necessary to use uuid here. Just create a random string of 10 characters and use it.

taranjeet · 2017-06-25T09:49:29Z

apps/challenges/views.py

+        # All files download and extract location.
+        BASE_LOCATION = tempfile.mkdtemp()
+        unique_folder_name = uuid.uuid4()
+        CHALLENGE_ZIP_DOWNLOAD_LOCATION = os.path.join('{}/{}.zip'.format(BASE_LOCATION, unique_folder_name))


Since os.path.join is used in many a place, you can simply import join by

from os.path import join

One more thing, since you are already using join, so need of using /. You can simply wrie

CHALLENGE_ZIP_DOWNLOAD_LOCATION = join(BASE_LOCATION, '{}.zip'.format(unique_folder_name))

taranjeet · 2017-06-25T09:51:39Z

apps/challenges/views.py

+
+    except:
+        response_data = {
+            'error': 'Some error occured during file upload. Please upload it again.'


Can we make the message to An error occured while uploading file again. Please try uploading it again.

taranjeet · 2017-06-25T09:53:01Z

apps/challenges/views.py

+        zip_ref = zipfile.ZipFile(CHALLENGE_ZIP_DOWNLOAD_LOCATION, 'r')
+        zip_ref.extractall('{}/{}'.format(BASE_LOCATION, unique_folder_name))
+        zip_ref.close()
+    except:


Can we be more specific here and catch only BadZipfile exception?

taranjeet · 2017-06-25T10:15:36Z

apps/challenges/views.py

+                    if os.path.isfile(test_annotation_file_path):
+                        challenge_test_annotation = open(test_annotation_file_path, 'rb')
+                        challenge_test_annotation_file = File(challenge_test_annotation)
+                        challenge_phase = ChallengePhase.objects.get(pk=challenge_phase.pk)


Please catch this in try-catch block.

Since this block of code is under transaction.atomic, it will automatically go into except block on line 571. So, should we still catch it here?

taranjeet · 2017-06-25T10:16:46Z

apps/challenges/views.py

+            # Create Challenge Phase Splits
+            yaml_file_data_of_challenge_phase_splits = yaml_file_data['challenge_phase_splits']
+            for data in yaml_file_data_of_challenge_phase_splits:
+                challenge_phase = challenge_phase_ids[data['challenge_phase_id']-1]


What is the logic of using -1 here.

The logic for -1 here is that since I am storing all the saved Leaderboards, Challenge Phases, Dataset splits in a list and the list indexing starts from 0 and the user will input the fields starting from 1,2 & so on. So, I reduced it by 1 to match the list index with user's input. Please tell if I am not clear.

You can create a dict here.

taranjeet · 2017-06-25T10:17:28Z

apps/challenges/views.py

+        if zip_config:
+            zip_config.challenge = challenge
+            zip_config.save()
+            response_data = {'success': 'The Challenge is successfully created'}


Please pass id in the message and remove the

taranjeet · 2017-06-25T10:17:53Z

apps/challenges/views.py

+            response_data = {'success': 'The Challenge is successfully created'}
+            return Response(response_data, status=status.HTTP_201_CREATED)
+    except:
+        response_data = {'error': 'Error in challenge creation. Please try again'}


Can we update the message to Error in creating challenge. Please try again.

taranjeet · 2017-06-25T10:20:11Z

apps/challenges/views.py

+        response_data = {'error': 'Error in challenge creation. Please try again'}
+        return Response(response_data, status=status.HTTP_400_BAD_REQUEST)
+
+    try:


Use shutil.rmtree for deleting folder. Also logger are for your help. We can make them more meaningful by making them more specific like adding ids or other data. So here in case folder is successfully deleted, no need of logging the message but please log proper specific message in case it fails to remove.

RishabhJain2018 · 2017-06-26T08:53:43Z

@taranjeet @deshraj I have fixed most of the review changes, but some require a discussion so, I will fix those after tonight's discussion.

deshraj · 2017-07-01T17:26:06Z

apps/challenges/urls.py

@@ -25,4 +25,7 @@
        name='get_challenges_based_on_teams'),
    url(r'(?P<challenge_pk>[0-9]+)/challenge_phase_split$', views.challenge_phase_split_list,
        name='challenge_phase_split_list'),
+    url(r'challenge/challenge_host_team/(?P<challenge_host_team_pk>[0-9]+)/zip-upload/$',


As far as I remember, we settled with using Underscores in the URLs and not hyphens, right?

deshraj · 2017-07-01T17:27:10Z

apps/challenges/views.py

+            with open(CHALLENGE_ZIP_DOWNLOAD_LOCATION, 'w') as zip_file:
+                zip_file.write(response.content)
+
+    except:


Lets not make the exception to be generic. Please handle the proper exceptions.

deshraj · 2017-07-01T17:27:31Z

apps/challenges/views.py

+        zip_ref = zipfile.ZipFile(CHALLENGE_ZIP_DOWNLOAD_LOCATION, 'r')
+        zip_ref.extractall(join(BASE_LOCATION, unique_folder_name))
+        zip_ref.close()
+    except BadZipfile:


This looks good. :)

deshraj · 2017-07-01T17:28:01Z

apps/challenges/views.py

+        zip_ref.close()
+    except BadZipfile:
+        response_data = {
+            'error': 'The zip file contents cannot be extracted. Please check the format.'


Change the error message to The zip file contents cannot be extracted. Please check the format!

deshraj · 2017-07-01T17:29:33Z

apps/challenges/views.py

+
+    if (yaml_file_count > 1) or (yaml_file_count == 0):
+        response_data = {
+            'error': 'There are no or more than one yaml file in zip folder.'


Can we have two different error statements for these two conditions?

deshraj · 2017-07-01T17:30:21Z

apps/challenges/views.py

+    else:
+        response_data = {
+            'error': 'There is no key for evaluation script in yaml file.'\
+                      'Please add a key and then try uploading it again.'


Replace . with ! at the end.

deshraj · 2017-07-01T17:30:45Z

apps/challenges/views.py

+    else:
+        response_data = {
+            'error': 'No evaluation script is present in the zip file.'\
+                     'Please try uploading again the zip file after adding evaluation script.'


Replace . with ! at the end.

deshraj · 2017-07-01T17:31:07Z

apps/challenges/views.py

+            response_data = {
+                'error': 'There is no key for test annotation file for'\
+                         'challenge phase {} in yaml file.'\
+                         'Please add a key and then try uploading it again.'.format(data['name'])


Replace . with ! at the end.

deshraj · 2017-07-01T17:31:22Z

apps/challenges/views.py

+            response_data = {
+                'error': 'No test annotation file is present in the zip file'\
+                         'for challenge phase {} Please try uploading'\
+                         'again the zip file after adding test annotation file.'.format(data['name'])


Replace . with ! at the end.

deshraj · 2017-07-01T17:33:12Z

apps/challenges/views.py

+                                                     save=True)
+
+            # Create Leaderboard
+            yaml_file_data_of_leaderboard = yaml_file_data['leaderboard']


Also even if there are more serializers, I think in that case we can create a serializer which simply takes list as argument and then saves it. See ChallengeParticipantTeamListSerializer in the code base.

@taranjeet you are right here.

deshraj · 2017-07-02T19:00:15Z

apps/challenges/serializers.py

    class Meta:
        model = Challenge
        fields = ('id', 'title', 'short_description', 'description', 'terms_and_conditions',
-                  'submission_guidelines', 'start_date', 'end_date', 'creator',
-                  'published', 'enable_forum', 'anonymous_leaderboard', 'is_active',)
+                  'submission_guidelines', 'start_date', 'end_date', 'creator', 'evaluation_details',


@RishabhJain2018 can you please tell me what is the significance of evaluation_details here? I might be missing something here.

Actually, evaluation_criteria and evaluation_details are same fields. In models we have defined it by evaluation_details. So, I am using evaluation_details to represent evaluation_criteria here.

deshraj · 2017-07-02T19:01:07Z

apps/challenges/views.py

+                    zip_file.write(response.content)
+        except IOError:
+            response_data = {
+                'error': 'Unable to process the uploaded zip file. Please upload it again! '


No space after !

deshraj · 2017-07-02T19:01:45Z

apps/challenges/views.py

+
+    except requests.exceptions.RequestException:
+        response_data = {
+            'error': 'A server error occured while processing zip file. Please try uploading it again.'


Replace with 'error': 'A server error occurred while processing zip file. Please try uploading it again!'

deshraj · 2017-07-02T19:02:59Z

apps/challenges/views.py

+
+    if not yaml_file_count:
+        response_data = {
+            'error': 'There is no yaml file in zip folder.'


Replace with 'error': 'There is no YAML file in zip configuration you provided!'

deshraj · 2017-07-02T19:04:00Z

apps/challenges/views.py

+
+    if yaml_file_count > 1:
+        response_data = {
+            'error': 'There are more than one yaml file in zip folder.'


Replace with 'error': 'There are more than one YAML files in zip folder!'

deshraj · 2017-07-02T19:04:57Z

apps/challenges/views.py

+
+    # Check for challenge image in yaml file.
+    image = yaml_file_data['image']
+    if image.endswith('.jpg') or image.endswith('.jpeg') or image.endswith('.png'):


Please make sure that we mention in our documentation that the logo can be of jpg/jpeg/png type only.

@deshraj yeah sure. I will take a note of it.

deshraj · 2017-07-02T19:06:43Z

tests/unit/challenges/test_views.py

+
+        response = self.client.post(self.url, {})
+        self.assertEqual(response.data.values()[0], expected['error'])
+        self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED)


I am sure that there are a lot of test cases that we are missing out here.

RishabhJain2018 · 2017-07-04T08:31:51Z

@taranjeet @deshraj Please review the PR. Also guide me to write the tests for some cases. I am stuck here.

deshraj · 2017-07-12T20:56:54Z

tests/unit/participants/test_models.py


    def test__str__(self):
        self.assertEqual(self.participant_team.team_name,
                         self.participant_team.__str__())
-
-    def test_get_all_participants_email(self):


Why are we removing this test?

RishabhJain2018 force-pushed the zip-challenge-views branch 2 times, most recently from 18cea0a to 73481ad Compare June 12, 2017 12:39

RishabhJain2018 changed the title ~~Add Challenge creation using zip file.~~ [REVIEW] Add Challenge creation using zip file. Jun 20, 2017

RishabhJain2018 changed the title ~~[REVIEW] Add Challenge creation using zip file.~~ [REVIEW PENDING] Add Challenge creation using zip file. Jun 20, 2017

taranjeet reviewed Jun 21, 2017

View reviewed changes

RishabhJain2018 force-pushed the zip-challenge-views branch from c60995b to 9c17086 Compare June 24, 2017 23:01

RishabhJain2018 force-pushed the zip-challenge-views branch from 0589018 to 8dd4959 Compare June 25, 2017 07:18

taranjeet reviewed Jun 25, 2017

View reviewed changes

RishabhJain2018 force-pushed the zip-challenge-views branch from 8dd4959 to 4962dca Compare June 26, 2017 08:52

deshraj requested changes Jul 1, 2017

View reviewed changes

deshraj reviewed Jul 2, 2017

View reviewed changes

RishabhJain2018 mentioned this pull request Jul 3, 2017

Modify challenge serializers to accept file input. #1076

Merged

RishabhJain2018 force-pushed the zip-challenge-views branch from 3e6265b to fe09682 Compare July 4, 2017 08:27

RishabhJain2018 force-pushed the zip-challenge-views branch from c14f397 to 43f8433 Compare July 12, 2017 20:37

deshraj approved these changes Jul 12, 2017

View reviewed changes

deshraj reviewed Jul 12, 2017

View reviewed changes

Flake8 fixes.

b958314

RishabhJain2018 force-pushed the zip-challenge-views branch from 842b608 to b958314 Compare July 12, 2017 21:11

Examples added.

daab401

deshraj merged commit 86bfa28 into Cloud-CV:master Jul 12, 2017

RishabhJain2018 deleted the zip-challenge-views branch July 13, 2017 19:34

deshraj pushed a commit that referenced this pull request Oct 24, 2018

Challenges: Add Challenge creation using zip configuration (#1037)

6f6a5e7

[REVIEW PENDING] Add Challenge creation using zip file. #1037

[REVIEW PENDING] Add Challenge creation using zip file. #1037

Conversation

RishabhJain2018 commented Jun 12, 2017 • edited Loading

taranjeet commented Jun 20, 2017

RishabhJain2018 commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RishabhJain2018 commented Jun 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RishabhJain2018 Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RishabhJain2018 commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RishabhJain2018 commented Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

RishabhJain2018 commented Jun 12, 2017 •

edited

Loading

RishabhJain2018 Jun 26, 2017 •

edited

Loading

RishabhJain2018 commented Jul 4, 2017 •

edited

Loading