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

Autolab partial feedback endpoints #1645

Merged
merged 72 commits into from
Dec 4, 2022
Merged

Conversation

michellexliu
Copy link
Contributor

@michellexliu michellexliu commented Nov 16, 2022

Description

Calls the Tango partial output and live jobs endpoints from Autolab to render more detailed job status and display partial output.

New feedback view states

In Progress submissions:
Screen Shot 2022-11-29 at 1 41 43 AM
Users are able to press a refresh button to get updated partial output, which gets displayed in the feedback section. The refresh button is rate-limited from the frontend such that users may only call a refresh every 5 seconds. If there is no longer any partial output, the refresh will trigger a page reload to check for full output.

Queued submissions:
Screen Shot 2022-11-29 at 1 41 11 AM
If a submission is queued, a "queued" page will get displayed, as well as the student's queue position. There is similarly a rate-limited refresh button.

Completed submissions:
Once autograding is complete, the feedback is displayed as normal, with no refresh button.

Notable behavior changes

When the autograder is still running, the -- in the grades table temporarily becomes clickable. If users click into it, they are led to the view feedback page, where the possible views mentioned above are shown.
image

After the autograder finishes running, the auto-graded scores are updated to numbers and remain clickable. Non-auto-graded scores go back to the original behavior.
image

If a user has clicked into the viewFeedback page of a non-auto-graded problem while the file was still being autograded, and then refreshes on it after autograding is complete, they will be redirected with a "No feedback for requested score" error.

Motivation and Context

This change allows students to view feedback as soon as it is updated, rather than just at the end. This is especially important for classes with long test cases, where students may not want to wait until their code is completely done to be able to see individual test case results.

How Has This Been Tested?

Setup

  1. Run Tango (redis-server, python3 restful_tango/server.py, python jobManager.py)
  2. Created assignment with Python autograder
  3. Create Python files that print every second for 30 seconds, 60 seconds, 90 seconds, etc.

Test scenarios:

  • Standard autograder feedback complete view: displayed as "complete", no refresh button, feedback loads
  • Standard autograder feedback complete view with an autograder error: displayed as "complete", no refresh button, feedback loads
  • Manual feedback: displayed as "complete", no refresh button, feedback loads
  • In-progress submission: loads properly, refresh button works
  • Queued submission: successfully shows queue position, refresh button works
  • In-progress -> in-progress refresh, In-progress -> finished running, in-progress -> finished running with Autograder error, queued -> finished running with semantic config
  • Queued -> updated queued position refresh, queued -> in-progress, queued -> finished running, queued -> finished running with Autograder error, queued -> finished running with semantic config
  • Output with semantic configuration
  • All of the above in both student view & instructor view

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run rubocop for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting
  • My change requires a change to the documentation, which is located at Autolab Docs
  • I have updated the documentation accordingly, included in this PR

} else if (!res.is_assigned) {
/* if there's no partial feedback and no queue position,
refresh the page to see if full feedback is available */
location.reload();
Copy link
Contributor

@victorhuangwq victorhuangwq Dec 2, 2022

Choose a reason for hiding this comment

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

I think this is the line that is causing the refresh loop. Basically I think my tango is acting weird (partialfeedback endpoint was not setup properly, so it doesn't work), and thus there is no partial feedback and no queue position.

Copy link
Contributor

@victorhuangwq victorhuangwq Dec 2, 2022

Choose a reason for hiding this comment

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

I have figured out how to replicate this bug:

  • Set up Tango without the PR that introduces partial feedback
  • Submit an assignment normally
  • Because partial feedback isn't working (the endpoint doesn't exist), so it gives
    image

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically, there should be error handling when the partial feedback endpoint throws an error.
Here's what currently happens when autograding fails (other parts of it)
image

Copy link
Contributor

@victorhuangwq victorhuangwq left a comment

Choose a reason for hiding this comment

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

Tested locally. Works well for the ideal use case.

Requested changes for:

  • Spinning behavior of refresh on click
  • Better error handling in the Autolab's partial feedback endpoint and in the refreshPartial function

Copy link
Contributor

@victorhuangwq victorhuangwq left a comment

Choose a reason for hiding this comment

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

LGTM.
Tested locally, with both old tango and new tango (with partial output) and works as expected.

@damianhxy
Copy link
Member

Tangentially related:

With the removal of the max-width: 970px in the previous streaming output PR, I observed the following issue where the handin form is too "narrow"

Screenshot 2022-12-02 at 23 20 55

This is because the div has a col style applied without any modifier. We should add a s12 class as follows in assessments/_handin_form.html.erb

Screenshot 2022-12-02 at 23 21 18

So that it renders properly

Screenshot 2022-12-02 at 23 22 37

Note: I did a grep and this seems to be the only instance of a class="col"

Copy link
Member

@damianhxy damianhxy left a comment

Choose a reason for hiding this comment

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

Followed testing plan, semantic config works too, code generally looks good

Requesting the following changes

  • Add s12 to handin form div
  • Update logic for @problemReleased
  • Remove extraneous schema changes
  • Address off-center rotation of refresh button

Commented some other nits too

<p>
Press the refresh button to get updates.
</p>
<% else %>
Copy link
Member

Choose a reason for hiding this comment

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

Nit: The format for an autograder success seems pretty different from the format for an autograder error / manual grading:

Success:
Screenshot 2022-12-02 at 23 29 35

Manual grading
Screenshot 2022-12-02 at 23 34 10

Error:
Screenshot 2022-12-02 at 23 33 19

Could we make the formats consistent?

(Rough mockup)
Screenshot 2022-12-02 at 23 37 45

app/views/assessments/_results_panel.html.erb Outdated Show resolved Hide resolved
db/schema.rb Show resolved Hide resolved
app/views/assessments/viewFeedback.html.erb Show resolved Hide resolved
} else if (res.queue_position != null) {
$('#partial-feedback').html(`
<div class="autograding-in-progress">
<pre>You are at position ${(res.queue_position) + 1} / ${(res.queue_length)} in the autograding queue.</pre>
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Do students need to know / care about the queue_length? Perhaps displaying only queue_position would be sufficient.

Your submission is currently in the autograding queue, waiting to be graded.
</p>
<p>
Press the refresh button to get updates.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Click is more semantically correct than press for a software button

app/views/assessments/_results_panel.html.erb Outdated Show resolved Hide resolved
app/views/assessments/_results_panel.html.erb Outdated Show resolved Hide resolved
Copy link
Member

@damianhxy damianhxy left a comment

Choose a reason for hiding this comment

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

Other than nits, LGTM

@damianhxy damianhxy linked an issue Dec 4, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grade should be auto-refreshed
3 participants