Add contest exporter #142

Holmes98 · 2022-01-15T22:19:50Z

No description provided.

tom93

I pushed a couple of proposed changes (completely untested), let me kow what you think:

add-contest-exporter-v3a-small-fixes-wip @ c11d30e [multiple commits]
add-contest-exporter-v3b-refactor-get-submissions-wip @ d51c5df
add-contest-exporter-v3c-efficient-submissions-export-wip @ 88786ea

What are the use cases for the export feature? The format is very rough:

The JSON files are a raw database dump using the internal representation, which is not user-friendly and seems inconvenient when using from a script. Would it be worthwhile to design a better format?
The submissions are currently in a single flat directory (with the submission ID as the filename), which is not user-friendly. It would be much easier to browse if they were organised into subdirectories by user and problem.

Would it be useful to create separate download links for just scoreboard.csv (and unofficial_scoreboard.csv)? (Most of the files in the zip file are machine-readable and not user friendly, while the scoreboard CSV is pretty much the opposite; so to me it makes sense to separate them.)

I'm curious about the commit 1d4e205 "Disallow organisers from exporting contests", what's the reasoning behind that?

tom93 · 2023-02-07T10:57:21Z

app/models/contest.rb

+  def get_submissions(user_id, problem_id)
+    relation = self.contest_relations.where(:user_id => user_id).first
+    Submission.where(:user_id => user_id, :problem_id => problem_id, :created_at => relation.started_at..relation.finish_at)
+  end


Note that this fails if the user is not in the contest (relation will be nil so relation.started_at will fail).
I'm not sure if that's good or bad; I'm inclined to leave it as is (even though it's slightly inconsistent with the other methods, which return nil).

tom93 · 2023-02-07T11:09:07Z

app/exporters/contests/contest_exporter.rb

+        contest.scoreboard.each do |record|
+          is_ranked = !contest.only_rank_official_contestants || (record.school && record.school_year)
+          next unless is_ranked || include_unranked_contestants
+
+          if is_ranked
+            num_ranked += 1
+            rank = num_ranked unless previous_record && previous_record.score == record.score && previous_record.time_taken == record.time_taken
+            previous_record = record
+          end


This complex logic is the same as in app/views/contests/scoreboard.html.erb, it should really be refactored.
But it can wait for separate PR in the interest of moving forward with other things.

tom93 · 2023-02-07T11:10:03Z

app/views/contests/_admin.html.erb

+<% if policy(Contest).export? %>
+  <br>
+  <b> Export Contest</b>
+  <%= form_tag( export_contest_path(@contest), :method => :get) do %>
+    <%= submit_tag "Download" %>
+  <% end %>
+<% end %>
+


I'd prefer for the export UI and the existing admin UI to move into "info" tab (#123 (comment)), I'll make a separate PR.

tom93 · 2023-02-07T11:11:52Z

app/exporters/contests/contest_exporter.rb

+      contest.scoreboard.each do |record|
+        next if record.user.nil?
+        contest.problem_set.problems.each do |prob|


There is no need to compute the scoreboard for getting the users and submissions, we can just use
contest.contestant_records.pluck(:user_id).compact
(though actually that might incur an extra DB query if the query from the scoreboard method is cached... need to investigate).
Ideally we'd save the scoreboard in local variable regardless (used twice above).

tom93 · 2023-02-07T11:13:44Z

app/exporters/contests/contest_exporter.rb

+          submissions += contest.get_submissions(record.user.id, prob.id)
+        end


Nit: The underlying query in get_submissions supports an array of problems/IDs, so we can use one query per user:

submissions += contest.get_submissions(record.user.id, contest.problem_set.problems)

(This is a little sneaky. We could rename the parameter problem_id of get_submissions to problem_ids or problems to document this.)

app/exporters/contests/contest_exporter.rb

Holmes98 · 2023-02-08T06:00:51Z

Thanks for reviewing. The proposed changes in add-contest-exporter-v3c-efficient-submissions-export-wip all look reasonable to me; I can try testing them in the weekend.

This feature is pretty much only being used for NZIC, as far as I'm aware. The JSON files are intended to provide a dump of all the contest metadata that could be used for various scripts (e.g. to generate statistics, detect cheating), but they do include some sensitive data (full names, email addresses) which organisers don't have access to, which was the reason for 1d4e205.

We already have a few scripts that work with the current format so I'd prefer to leave them as is, but I wouldn't mind if we changed the directory structure for submissions to /{user_id}/{problem_id}/{submission_id}.{extension} or something similar. I also wouldn't be opposed to having extra download links for just the scoreboards, but it wouldn't make a difference for NZIC since we'd always do a full export.

The directories directly under app/ are root directories and their names aren't part of the namespace. So app/exporters/base_exporter.rb should implement BaseExporter, not Exporters::BaseExporters. See: https://guides.rubyonrails.org/autoloading_and_reloading_constants.html#introduction https://guides.rubyonrails.org/autoloading_and_reloading_constants.html#config-autoload-paths

Whe selecting the submissions made during the contest, change the time interval from `..` (inclusive-inclusive) to `...` (inclusive-exclusive) for consistency with the existing logic in other places (e.g. app/models/contest_score.rb has a very similar query).

…contest exporter They might be different if the user changes school after the contest.

… refactor

The previous approach loaded the source code of all the submissions into memory at once, which may be problematic.

tom93

I tested my proposed changes, fixed a typo (5be2eb0), then rebased and pushed to this PR's branch.

I'm approving this version, and will use separate PRs for the bigger changes.

I propose to squash-and-merge with the following commit message:

Add contest exporter (#141)

Co-authored-by: Tom Levy <[email protected]>

The "deployed" branch currently has several commits that are not in "master". They consist of the first commit from this PR, and several temporary commits for email workarounds that have been reverted.

The server is at the "deployed" branch but also an uncommitted patch to fix a divide-by-zero in the progress bars; I created #160 to mirror that patch ~~(+ small refactoring)~~.

So if we merge both PRs, we can safely hard-reset the "deployed" branch (and the server) to "master" without losing anything. The only changes that will be introduced are small fixes and refactoring.

We already have a few scripts that work with the current format so I'd prefer to leave them as is

The biggest issue I have with the current representation is how the individual scores are stored (details below). I'd very much like to change that (in a separate PR). The scripts will need to be tweaked a little, I hope that's not a problem; I'd be happy to help.

Currently the ContestScore fields (which are per-problem-per-user) are embedded in the parent ContestRelation object (which is per-user), with the problem ID appended to each field name. Example (pseudo-JSON, with two problems whose IDs are 111 and 222):

scoreboard: [
    {
        user_id, score, time_taken, school_id,
        score_111, attempt_111, attempts_111, sub_111,
        score_222, attempt_222, attempts_222, sub_222
    },
    {
        user_id, ...
    }
]

I think it would be much nicer (both for the app's code and for the scripts) if the ContestScore objects were stored in an array, in the same order as the problems:

scoreboard: [
    {
        user_id, score, time_taken, school_id,
        scores: [
            { score, attempt, attempts, sub },
            { score, attempt, attempts, sub }
        ]
    },
    {
        user_id, ...
    }
]

Such an array can be produced using the following snippet:

contest_relation.contest_scores.index_by(&:problem_id).values_at(*problem_ids)

Another option is to store them in a hash, with the problem ID as the key. This corresponds very closely to the old representation.

Yet another option (which I really don't like) is to use an unordered array of ContestScore objects. I don't like it because it is awkward to use from scripts, but it is worth mentioning because it directly corresponds to the existing contest_scores association.

Holmes98 · 2023-02-10T05:54:23Z

The biggest issue I have with the current representation is how the individual scores are stored (details below). I'd very much like to change that (in a separate PR).

Oh sure, feel free to make changes to anything under the contest JSON. I thought it could be useful at some point but we're currently only using the submissions JSON.

tom93 · 2023-02-10T06:27:49Z

Shall I deploy? (Hard-reset etc.)

Holmes98 · 2023-02-10T06:28:18Z

Sure

tom93 · 2023-02-10T07:13:22Z

Done!

Co-authored-by: Tom Levy <[email protected]>

Holmes98 added 2 commits March 6, 2021 13:23

Add contest exporter

5278ff4

Disallow organisers from exporting contests

1d4e205

tom93 requested changes Feb 7, 2023

View reviewed changes

bagedevimo reviewed Feb 7, 2023

View reviewed changes

app/exporters/contests/contest_exporter.rb Show resolved Hide resolved

app/exporters/contests/contest_exporter.rb Show resolved Hide resolved

tom93 mentioned this pull request Feb 7, 2023

Reduce DB queries for schools when using scoreboard [small] #151

Open

tom93 added 6 commits February 10, 2023 05:17

Remove unused include RenderAnywhere in contest exporter

7f2e892

Use ContestRelation.school instead of ContestRelation.user.school in …

d087379

…contest exporter They might be different if the user changes school after the contest.

Move implementation of Contest.get_submissions to ContestRelation and…

469f777

… refactor

Write submissions in chunks to use less memory when exporting contest

353a28f

The previous approach loaded the source code of all the submissions into memory at once, which may be problematic.

tom93 approved these changes Feb 9, 2023

View reviewed changes

Holmes98 merged commit c741e3e into master Feb 10, 2023

Holmes98 deleted the add-contest-exporter-v2 branch February 10, 2023 06:04

bagedevimo pushed a commit to bagedevimo/nztrain that referenced this pull request Feb 10, 2023

Add contest exporter (NZOI#142)

71b02e9

Co-authored-by: Tom Levy <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add contest exporter #142

Add contest exporter #142

Holmes98 commented Jan 15, 2022

tom93 left a comment

tom93 Feb 7, 2023

tom93 Feb 7, 2023

tom93 Feb 7, 2023

tom93 Feb 7, 2023

tom93 Feb 7, 2023

Holmes98 commented Feb 8, 2023

tom93 left a comment •

edited

Loading

Holmes98 commented Feb 10, 2023

tom93 commented Feb 10, 2023

Holmes98 commented Feb 10, 2023

tom93 commented Feb 10, 2023

		submissions += contest.get_submissions(record.user.id, prob.id)
		end

Add contest exporter #142

Add contest exporter #142

Conversation

Holmes98 commented Jan 15, 2022

tom93 left a comment

Choose a reason for hiding this comment

tom93 Feb 7, 2023

Choose a reason for hiding this comment

tom93 Feb 7, 2023

Choose a reason for hiding this comment

tom93 Feb 7, 2023

Choose a reason for hiding this comment

tom93 Feb 7, 2023

Choose a reason for hiding this comment

tom93 Feb 7, 2023

Choose a reason for hiding this comment

Holmes98 commented Feb 8, 2023

tom93 left a comment • edited Loading

Choose a reason for hiding this comment

Holmes98 commented Feb 10, 2023

tom93 commented Feb 10, 2023

Holmes98 commented Feb 10, 2023

tom93 commented Feb 10, 2023

tom93 left a comment •

edited

Loading