Submission export #77

jathak · 2015-06-06T01:28:09Z

Implements an option for course staff to export all final submissions for an assignment, as mentioned in this issue.

Adding the --export option to a command overrides the client's default behavior and instead downloads all final submissions for the provided assignment (the course is determined by the optional --course parameter or selected from a list of all courses if not provided).

albert12132 · 2015-06-06T04:57:43Z

The ExportProtocol doesn't fit the behavior of a Protocol, as far as I know; I thought the original intention of a Protocol was a routine that runs before network communication. In this case, ExportProtocol runs in place of network communication.

@soumyabasu , what do you think? We should either expand the definition of a protocol or reword ExportProtocol as something else.

albert12132 · 2015-06-06T05:01:27Z

client/protocols/export.py

+            print("Submissions downloaded.")
+            if os.path.exists('export_cache.pkl'):
+                os.remove('export_cache.pkl')
+            sys.exit(0)


I'd prefer if ok.py:main was the only function that uses exit to abort; that way, it's easier to keep track of all the ways an abort can occur. I suggest rewriting ok.py to do something like this:

if args.export: export.protocol... exit(0)

I agree with your preference, but I also don't like changing ok.py:main every time that we add a functionality like this. We should figure out how to nicely achieve both goals.

I don't see the issue with changing ok.py:main every time we want to add an abort. Can you elaborate?

I think there will be a family of protocols that do something like this: make API call to the server to either fetch or update some state and then exit. I don't want to keep adding things to ok-main every time that happens since that'll get cluttered.

It might be good to then subclass things? like your GradingProcotol and models framework?

soumyabasu · 2015-06-06T05:30:40Z

I was thinking of this as a definition: A Protocol is something that runs before a backup is taken.

I think that we should have a backup flag that some protocols (e.g. this one) can turn off to indicate that no backup should be taken on this run to ok.

albert12132 · 2015-06-06T05:33:56Z

Some care needs to be taken to specify which protocols are "special cases" (i.e. this protocol is explicitly mentioned in ok.py:main, whereas other protocols are just part of the for protocol in protocols section). In this particular case, the ExportProtocol would also want to prevent other protocols from running.

soumyabasu · 2015-06-06T05:41:00Z

Okay, how about a slightly different design? We have a 'NetworkProtocol' that's responsible for any server/user interaction. This protocol is one of the "special cases" you mentioned and downloading all final submissions for an assignment is just one thing that it can do.

albert12132 · 2015-06-06T05:43:38Z

The NetworkProtocol idea sounds better to me. We can have a switch statement in ok.py that selects what sort of network protocol to run based on command-line arguments.

soumyabasu · 2015-06-06T05:46:45Z

@jathak - What do you think about this?

jathak · 2015-06-06T05:51:35Z

That definitely makes sense. Would this still require updating ok.py:main for each new feature like this or could we make it so it looks through all available NetworkProtocols and chooses one automatically based on the command-line arguments?

jathak · 2015-06-06T06:02:07Z

Is it better to define features like this one as protocols that run before (or in place of) the backup or as protocols that run before loading the assignment configuration? For instance, if we added a feature for students to restore to an backup, it would need to run after loading the assignment but it would still need to make network requests in place of a backup.

soumyabasu · 2015-06-06T06:12:02Z

Would this still require updating ok.py:main for each new feature like this or could we make it so it looks through all available NetworkProtocols and chooses one automatically based on the command-line arguments?

Each protocol has access to command line arguments, so I think it would be fine to do it in either place. I'm preferential to ok.py:main just knowing that NetworkProtocol needs to be run and the NetworkProtocol picking the right thing to do. It's fine to just use your best judgment in cases like this and/or ping someone to talk about it.

Is it better to define features like this one as protocols that run before (or in place of) the backup or as protocols that run before loading the assignment configuration?

Before the backup for precisely the reason you mentioned. Also, you could make it so that the "export all submissions" thing just downloads all of the final submissions for the assignment that was loaded into ok as well.

jathak · 2015-06-06T06:55:42Z

Should NetworkProtocols only be available if included in the assignment configuration or should they be available regardless (provided the proper command-line argument is used)?

soumyabasu · 2015-06-06T07:04:12Z

I think it should be included in the assignment configuration to keep things consistent- it ultimately should be up to the instructors whether or not their students are allowed to use ok features.

albert12132 · 2015-06-06T07:19:38Z

NetworkProtocols can use a mechanism similar to regular protocols to
determine which one should be used; that way, ok.py:main won't have to be
modified every time. I can work on setting up the NetworkProtocol framework
in the next few days.

On Sat, Jun 6, 2015 at 3:04 AM, soumyabasu notifications@github.com wrote:

I think it should be included in the assignment configuration to keep
things consistent- it ultimately should be up to the instructors whether or
not their students are allowed to use ok features.

—
Reply to this email directly or view it on GitHub
#77 (comment)
.

albert12132 · 2015-06-06T08:46:09Z

We can merge in the export protocol functionality before I rewrite the
network protocol stuff, since I won't be able to work on it for a couple of
days.

On Sat, Jun 6, 2015, 3:19 AM Albert Wu albert12132@gmail.com wrote:

NetworkProtocols can use a mechanism similar to regular protocols to
determine which one should be used; that way, ok.py:main won't have to be
modified every time. I can work on setting up the NetworkProtocol framework
in the next few days.

On Sat, Jun 6, 2015 at 3:04 AM, soumyabasu notifications@github.com
wrote:

I think it should be included in the assignment configuration to keep
things consistent- it ultimately should be up to the instructors whether or
not their students are allowed to use ok features.

—
Reply to this email directly or view it on GitHub
#77 (comment)
.

albert12132 · 2015-06-16T21:37:03Z

client/protocols/export.py

+        data = None
+        try:
+            data = pickle.load(open("export_cache.pkl", "rb"))
+            self.access_token = auth.authenticate(self.args.authenticate)


This authentication should probably happen before the try/except block; it seems like you want it to happen regardless of whether or not there is an error.

albert12132 · 2015-06-16T21:47:43Z

client/protocols/export.py

+                pickle.dump(data, open("export_cache.pkl", "wb"))
+                dl_left = len(data['students']) - i
+                print("Download interrupted. Run command again to continue.")
+                print("{0} submissions left to download".format(dl_left))


Abstract this logic away into a subroutine def abort(data, total_students, current_student):, since you use it in multiple places.

Better yet, just catch the IOError and HTTPError here:

except (KeyboardInterrupt, IOError, HTTPError) as e: ... print("Download could not be completed. Run the following command to continue:") ...

Also, please include a log warning that prints the exception message and includes a stack trace:

log.warning("Incomplete download. %s: %s", type(e).__name__, str(e), exc_info=True)

albert12132 · 2015-06-16T21:54:00Z

client/utils/network.py

+import socket
+
+TIMEOUT = 500
+RETRY_LIMIT = 5


I don't think this is needed anymore.

albert12132 · 2015-06-16T22:05:36Z

Looks good so far. Have you been able to get this working?

jathak · 2015-06-16T22:09:33Z

I've tested this with a local server and it works.

albert12132 · 2015-06-16T22:15:56Z

Great. That's all the comments I have, so I'll just wait until you're done making changes. @soumyabasu , do you have any comments?

soumyabasu · 2015-06-17T06:01:50Z

Nope, I'm good with the high level design and I don't have anything additional to add to your code review.

jathak · 2015-06-17T06:07:45Z

I've resolved Albert's comments, so let me know if there's anything else that needs to be done before this can be merged.

albert12132 · 2015-06-17T06:32:33Z

client/protocols/export.py

+
+        current_student = 0
+        try:
+            if not os.path.exists('submissions'): 


"submissions" should be stored in a global variable at the top of the file (e.g. SUBMISSION_DIR = 'submissions'). You also use 'submissions' on line 82.

jathak · 2015-06-17T20:19:14Z

Should I increment the version number?

albert12132 · 2015-06-17T20:22:11Z

No, I'll do that on the master branch. Merging now.

Submission export

jathak added 2 commits June 5, 2015 17:22

preliminary support for submission export

ddfb4ba

minor fixes

bf235a3

albert12132 reviewed Jun 6, 2015
View reviewed changes

albert12132 mentioned this pull request Jun 6, 2015

Send 1 message to server per ok run #7

Closed

soumyabasu mentioned this pull request Jun 10, 2015

Program or web interface to download all submissions for an individual student okpy/ok#445

Closed

albert12132 reviewed Jun 16, 2015
View reviewed changes

minor fixes

23c274a

albert12132 reviewed Jun 16, 2015
View reviewed changes

specifies what command to re-run when download fails

cb20980

albert12132 reviewed Jun 16, 2015
View reviewed changes

client/utils/network.py

import socket

TIMEOUT = 500

RETRY_LIMIT = 5

Copy link

Contributor

albert12132 Jun 16, 2015

Choose a reason for hiding this comment

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

I don't think this is needed anymore.

code cleanup

09627a1

albert12132 reviewed Jun 17, 2015
View reviewed changes

moved submissions directory to global variable

ae59a04

albert12132 added a commit that referenced this pull request Jun 17, 2015

Merge pull request #77 from jathak/enhancement/jathak/submission-export

06a28cd

Submission export

albert12132 merged commit 06a28cd into okpy:master Jun 17, 2015

albert12132 mentioned this pull request Jun 17, 2015

Program or web interface to download all final submissions for an assignment okpy/ok#444

Closed

jathak deleted the enhancement/jathak/submission-export branch June 18, 2015 03:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Submission export #77

Submission export #77

jathak commented Jun 6, 2015

albert12132 commented Jun 6, 2015

albert12132 Jun 6, 2015

soumyabasu Jun 6, 2015

albert12132 Jun 6, 2015

soumyabasu Jun 6, 2015

soumyabasu commented Jun 6, 2015

albert12132 commented Jun 6, 2015

soumyabasu commented Jun 6, 2015

albert12132 commented Jun 6, 2015

soumyabasu commented Jun 6, 2015

jathak commented Jun 6, 2015

jathak commented Jun 6, 2015

soumyabasu commented Jun 6, 2015

jathak commented Jun 6, 2015

soumyabasu commented Jun 6, 2015

albert12132 commented Jun 6, 2015

albert12132 commented Jun 6, 2015

albert12132 Jun 16, 2015

albert12132 Jun 16, 2015

albert12132 Jun 16, 2015

albert12132 Jun 16, 2015

albert12132 commented Jun 16, 2015

jathak commented Jun 16, 2015

albert12132 commented Jun 16, 2015

soumyabasu commented Jun 17, 2015

jathak commented Jun 17, 2015

albert12132 Jun 17, 2015

jathak commented Jun 17, 2015

albert12132 commented Jun 17, 2015

Submission export #77

Submission export #77

Conversation

jathak commented Jun 6, 2015

albert12132 commented Jun 6, 2015

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

soumyabasu commented Jun 6, 2015

albert12132 commented Jun 6, 2015

soumyabasu commented Jun 6, 2015

albert12132 commented Jun 6, 2015

soumyabasu commented Jun 6, 2015

jathak commented Jun 6, 2015

jathak commented Jun 6, 2015

soumyabasu commented Jun 6, 2015

jathak commented Jun 6, 2015

soumyabasu commented Jun 6, 2015

albert12132 commented Jun 6, 2015

albert12132 commented Jun 6, 2015

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

albert12132 commented Jun 16, 2015

jathak commented Jun 16, 2015

albert12132 commented Jun 16, 2015

soumyabasu commented Jun 17, 2015

jathak commented Jun 17, 2015

Choose a reason for hiding this comment

jathak commented Jun 17, 2015

albert12132 commented Jun 17, 2015