add --zip option to zip the submission file prior to uploading it #52

queirozfcom · 2017-10-21T18:00:56Z

If option is passed, a zip file is created (using python zipfile module) with a random name (cache busting) and the zipped file is uploaded instead, to use less bandwidth. The zip file is deleted afterwards to avoid using up too much disk.

…oading it

floydwch

Thanks for the contribution!

And there are some feedbacks to the implementation, before the merge, let's discuss and make some updates on it. Thank you.

floydwch · 2017-10-22T23:32:21Z

kaggle_cli/submit.py

@@ -25,7 +25,7 @@ def get_parser(self, prog_name):
        parser.add_argument('-u', '--username', help='username')
        parser.add_argument('-p', '--password', help='password')
        parser.add_argument('-z', '--zip', type=self._str2bool, nargs='?', const=True, default=False,


Why don't you use the same argument setting as config.py?

i.e. parser.add_argument('-z', '--zip', help='zip the submission file before uploading?', action='store_true')

floydwch · 2017-10-22T23:33:22Z

kaggle_cli/config.py

@@ -98,7 +98,7 @@ def take_action(self, parsed_args):
        parsed_arg_dict = vars(parsed_args)

        if DATA_OPTIONS & set(
-            filter(lambda x: parsed_arg_dict[x], parsed_arg_dict)
+                filter(lambda x: parsed_arg_dict[x], parsed_arg_dict)


Is it an accident duplicated indent?

IDE issues I guess

floydwch · 2017-10-22T23:36:42Z

kaggle_cli/submit.py

@@ -35,7 +35,12 @@ def take_action(self, parsed_args):
        username = config.get('username', '')
        password = config.get('password', '')
        competition = config.get('competition', '')
-        zip = config.get('zip', False)
+        zip_flag = config.get('zip', 'no')


You can use getboolean method instead.

@floydwch but the config object here is just a python dict, it's not a ConfigParser object

This issue should be resolved at the config provider level, I'll address it.

floydwch · 2017-10-22T23:42:27Z

kaggle_cli/submit.py

@@ -46,7 +51,7 @@ def take_action(self, parsed_args):
        entry = parsed_args.entry
        message = parsed_args.message

-        archive_name = Submit._rand_str(10)+'.zip'
+        archive_name = Submit._rand_str(10) + '.zip'


Since the uploaded file's name will also show on the Kaggle's dashboard. A related name might be better. i.e. append the uuid or timestamp to the original submission file's name.

And another concern there is, for myself habit, the submission files are already suffixed a timestamp to distinguish the trial. Adding additional suffix is duplicated.

I agree. What if I prepend a short random string instead?

queirozfcom · 2017-10-23T00:43:44Z

Thanks for the feedback. I'll edit some things and remake the request

…oading it

queirozfcom · 2017-10-23T23:13:24Z

I've applied the changes you suggested (except for using config.getboolean(), because the object is not a ConfigParser, but a normal python dict)

floydwch

New feedbacks here. And please also update the README.md to explain the new function.

And I'll update the master branch to provide the coerced bool field. You can rebase to the master after I updated it.

floydwch · 2017-10-24T02:01:31Z

kaggle_cli/submit.py

@@ -35,7 +35,12 @@ def take_action(self, parsed_args):
        username = config.get('username', '')
        password = config.get('password', '')
        competition = config.get('competition', '')
-        zip = config.get('zip', False)
+        zip_flag = config.get('zip', 'no')


This issue should be resolved at the config provider level, I'll address it.

floydwch · 2017-10-24T02:08:59Z

kaggle_cli/submit.py

-            zip = True
-        else:
-            zip = False
+        zip = Submit._str2bool(zip_flag)


Overriding the built-in function name is discouraged. You can just apply the _str2bool to the zip_flag = config.get('zip', 'no') , i.e. zip_flag = Submit._str2bool(config.get('zip', 'no')).

But I'll address the boolean coercion issue at config provider level, you can get a boolean by config.get('zip') after I updated it.

The type coercion enhancement has been done, see 9158c36 and 551a1b2 .

You can now config.get('zip').

floydwch · 2017-10-24T02:09:53Z

kaggle_cli/submit.py

-        archive_name = Submit._rand_str(10) + '.zip'
+        archive_name = Submit._make_archive_name(entry)
+
+        # print(archive_name)


Incident comments here.

floydwch · 2017-10-24T02:18:54Z

kaggle_cli/submit.py

@@ -129,29 +130,38 @@ def take_action(self, parsed_args):
            os.remove(target_name)

    @staticmethod


Static method is not the best choice in such case. And an underscore prefix method indicates this method is private, which means it'll access the object's private fields, but it's not the case here. I suggest you extract the method to the module scope, i.e. move the _make_archive_name outside the Submit class but still in the submit.py.

floydwch · 2017-10-24T02:20:21Z

kaggle_cli/submit.py

+        else:
+            archive_name = original_basename+".zip"
+
+        # this is used to prevent caching issues


I still can't understand why it has caching issues. Can you explain the scenario?

I'm not sure about this but I think that when we upload two submissions (different files) with the same same (say, "mysubmission.csv"), this may trigger some form of caching in Kaggle

(They may only check the file name and, if the file name is something they have seen before, they may not even read the file).

Again, I'm not sure about this but since caching is done by many people on many websites, I though it would be a safe thing to do at little cost (just add a couple characters to the file name).

But I agree it's not essential.

Caching usually applied on read operation, caching a file upload would be considered a bug.

But I just tested if Kaggle caches the file? The answer is no.

I uploaded the benchmark model for the titanic competition and updated the file's content manually and uploaded again to see if it's cached. The scores returned by Kaggle were different in the two submissions. So basically, it's not an issue.

…oading it

queirozfcom · 2017-11-05T02:57:48Z

@floydwch the changes have been ready for a couple of days now... anything you want to add?

floydwch · 2017-11-05T03:44:21Z

@queirozfcom sorry, I'm busy now. I'll reply to you ASAP.

floydwch · 2017-11-06T13:12:10Z

@queirozfcom I added new feedback on the caching issue.

And another concern is, please rebase the commits to the master branch for better commit logs, don't use merge.

queirozfcom added 2 commits October 21, 2017 00:40

add --zip flag and support for zipping a submission file prior to upl…

b5946c6

…oading it

add support for setting the --zip option in the config

7b33aaa

floydwch suggested changes Oct 22, 2017

View reviewed changes

floydwch added 2 commits October 23, 2017 08:11

add warning when the quota of submission ran out

d3c68a4

use response content to warn the user

f0869ee

queirozfcom added 4 commits October 23, 2017 19:10

add --zip flag and support for zipping a submission file prior to upl…

f8a67f8

…oading it

add support for setting the --zip option in the config

5116008

Merge branch 'master' of github.com:queirozfcom/kaggle-cli

ff4a29a

made suggested changes (where applicable)

23bb924

floydwch suggested changes Oct 24, 2017

View reviewed changes

floydwch and others added 10 commits October 24, 2017 11:39

add type of config field

574019d

add config field coercion

9158c36

prepare zip field

551a1b2

add --zip flag and support for zipping a submission file prior to upl…

7f9b3c0

…oading it

add support for setting the --zip option in the config

bd0cfed

add --zip flag and support for zipping a submission file prior to upl…

9e5d725

…oading it

add support for setting the --zip option in the config

d17fe8c

made suggested changes (where applicable)

2bb95b4

merged

1b0ae53

updated README.md

be32ad4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add --zip option to zip the submission file prior to uploading it #52

add --zip option to zip the submission file prior to uploading it #52

queirozfcom commented Oct 21, 2017 •

edited

Loading

floydwch left a comment

floydwch Oct 22, 2017

queirozfcom Oct 23, 2017

floydwch Oct 22, 2017

queirozfcom Oct 23, 2017

floydwch Oct 22, 2017

queirozfcom Oct 23, 2017

floydwch Oct 24, 2017

floydwch Oct 22, 2017

queirozfcom Oct 23, 2017 •

edited

Loading

queirozfcom commented Oct 23, 2017

queirozfcom commented Oct 23, 2017

floydwch left a comment

floydwch Oct 24, 2017

floydwch Oct 24, 2017

floydwch Oct 24, 2017 •

edited

Loading

floydwch Oct 24, 2017

queirozfcom Oct 27, 2017

floydwch Oct 24, 2017

floydwch Oct 24, 2017

queirozfcom Oct 27, 2017 •

edited

Loading

floydwch Nov 6, 2017

queirozfcom commented Nov 5, 2017

floydwch commented Nov 5, 2017

floydwch commented Nov 6, 2017 •

edited

Loading

		@@ -129,29 +130,38 @@ def take_action(self, parsed_args):
		os.remove(target_name)

		@staticmethod

add --zip option to zip the submission file prior to uploading it #52

Are you sure you want to change the base?

add --zip option to zip the submission file prior to uploading it #52

Conversation

queirozfcom commented Oct 21, 2017 • edited Loading

floydwch left a 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

queirozfcom Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

queirozfcom commented Oct 23, 2017

queirozfcom commented Oct 23, 2017

floydwch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

floydwch Oct 24, 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

queirozfcom Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

queirozfcom commented Nov 5, 2017

floydwch commented Nov 5, 2017

floydwch commented Nov 6, 2017 • edited Loading

queirozfcom commented Oct 21, 2017 •

edited

Loading

queirozfcom Oct 23, 2017 •

edited

Loading

floydwch Oct 24, 2017 •

edited

Loading

queirozfcom Oct 27, 2017 •

edited

Loading

floydwch commented Nov 6, 2017 •

edited

Loading