Expose attempt number to python custom grader by jolyonb · Pull Request #18149 · openedx/openedx-platform

jolyonb · 2018-05-07T17:23:46Z

This PR exposes a student's attempt number to a python custom grader, so that the custom grader can implement a "diminishing returns" grading policy. The following OLX code for a problem demonstrates how the attempt number can be accessed by the grader.

<problem>
<script type="text/python" system_path="python_lib">
def gradeit(expect, ans, **kwargs):
    attempt = kwargs["attempt"]
    message = "This is attempt number {}".format(str(attempt))
    return {
        'input_list': [
            { 'ok': True, 'msg': message},
        ]
    }
</script>
<p>Any non-blank answer is correct.</p>
<customresponse cfn="gradeit" expect="(anything)" cfn_extra_args="attempt">
  <textline size="45" correct_answer="(anything)"/>
</customresponse>
</problem>

Special care is taken to pass the correct attempt number through to the grader when performing a regrade.

Mentions: @pdpinch

openedx-webhooks · 2018-05-07T17:23:56Z

Thanks for the pull request, @jolyonb! I've created OSPR-2391 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

supporting documentation
edx-code email threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

mduboseedx · 2018-05-08T14:09:03Z

jenkins ok to test

jolyonb · 2018-05-08T14:44:31Z

jenkins run bokchoy

mduboseedx · 2018-05-08T15:44:54Z

@edx/educator-all Could you take a look at this?

pdpinch · 2018-07-01T16:07:01Z

jenkins run all

pdpinch · 2018-07-01T21:35:01Z

@jolyonb in writing tests for this, I may have stumbled onto a bug. Should the "attempt" count still be incremented when the student response raises an error (e.g. when there is a codejail exception)?

You can see this manifest in https://build.testeng.edx.org/job/edx-platform-python-unittests-pr/54182/testReport/junit/xmodule.xmodule.tests.test_capa_module/CapaModuleTest/test_submit_problem_error_with_codejail_exception/

jolyonb · 2018-07-01T21:42:23Z

This should be 2, not 1. What's happening is that the student is submitting attempt #2, and the grader needs to know that this is attempt number 2, not attempt number 1.

module.attempts points to the number of attempts USED

module.lcp.context['attempt'] points to the attempt number that is ABOUT to be used (or was just ATTEMPTED to be used)

pdpinch · 2018-07-03T10:53:41Z

@jolyonb tests are passing now, but I'm not convinced I have the correct combination of module.attempts and module.lcp.context['attempt']. There's probably also a couple of cases where I'm unnecessarily testing module.lcp.context['attempt'].

Can you take a look at the changes in test_capa_module.py and let me know if everything is what you expect? Also, I'm open to suggestions on how to make the comments clearer.

jolyonb · 2018-07-03T11:41:36Z

This one is just first attempt (not still same)

@jolyonb Does your comment mean the code comment is confusing?
It's really confusing to me when your PR comment contradicts the code comment you made.

The comment here is confusing; a complete comment would be "Expect that the number of attempts has incremented from 0 to 1, and that the context believes this was attempt 1". On subsequent grades in this function, a regrade is called, which should not increment the attempt number, and hence the context should still think it's attempt 1.

jolyonb · 2018-07-03T11:55:35Z

@pdpinch This needs a test to show that the variable is being passed into the check function. I believe this should be put in common/lib/capa/capa/tests/test_responsetypes.py, and you can basically copy the function test_function_code_with_extra_args with modifications as described in the details of this PR.

jolyonb · 2018-08-03T22:00:08Z

jenkins run bokchoy

pdpinch · 2018-08-06T17:07:25Z

@mduboseedx this is ready again. We added another test.

mduboseedx · 2018-08-06T20:35:12Z

@schenedx I believe this is assigned to your team. Could you or someone else give this a review?

jolyonb · 2018-08-20T20:48:44Z

Bump...

mduboseedx · 2018-08-21T18:59:37Z

@jolyonb Much like #18158, our Educator team will unable to do any reviews for a few weeks. I'm sorry for the delay, but I'll try to get another group to give this a review.

pdpinch · 2018-08-27T16:32:10Z

@mduboseedx this is rebased, is passing tests and I give it a 👍. Can I merge it?

schenedx

@pdpinch @jolyonb I have a few questions on this PR. Can you guys respond? I am fine with this feature since the code change is super simple.

schenedx · 2018-08-27T17:00:52Z

                }

        try:
+            self.lcp.context['attempt'] = self.attempts + 1


This is just weird to me. Why do we assign the attempt value to the lcp.context before we updated self.attempts counter on line 1221?
Does self.lcp.grade_answers use this key? if it does, why not just move the self.attempts = self.attempts + 1 line up?

The whole point of this is to attach the current attempt number to self.lcp.context so that a custom python grader called from self.lcp.grade_answers can access it. Note that if the grader raises an exception (such as unable to parse input etc), the attempt number self.attempts is not incremented. So, on the first attempt, the attempt counter starts at 0, and only increments if a non-exception-raising attempt occurs. However, for the purpose of the grader, that first attempt should be treated as attempt one. Similarly for later attempts.

@jolyonb I see. Then can you add some comment here to spell out that self.lcp.context['attempt'] is 1 based, but the self.attempts is 0 based? This will help with the person who were as confused as I am.

schenedx · 2018-08-27T17:02:10Z

@jolyonb Does your comment mean the code comment is confusing?
It's really confusing to me when your PR comment contradicts the code comment you made.

…egraded; Update comments

edx-status-bot · 2018-08-30T20:57:11Z

Your PR has finished running tests. There were no failures.

jolyonb · 2018-08-30T21:03:43Z

@schenedx This has been updated with further comments.

mduboseedx · 2018-09-10T19:38:26Z

@schenedx Do you think your comments have been addressed?

schenedx

👍 I'll merge now

openedx-webhooks · 2018-09-10T19:50:35Z

@jolyonb 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2018-09-10T20:37:26Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Tuesday, September 11, 2018.

edx-pipeline-bot · 2018-09-10T20:46:33Z

EdX Release Notice: This PR may have caused e2e tests to fail on Stage. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/STAGE_edxapp_M-D

jolyonb · 2018-09-10T22:38:59Z

I can't seem to access that link on the E2E test fail?

edx-pipeline-bot · 2018-09-11T15:01:44Z

EdX Release Notice: This PR has been deployed to the production environment.

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels May 7, 2018

openedx-webhooks added awaiting prioritization and removed needs triage labels May 8, 2018

jolyonb commented Jul 1, 2018

View reviewed changes

jolyonb commented Jul 3, 2018

View reviewed changes

pdpinch changed the title ~~Expose attempt number to python custom grader (WIP)~~ Expose attempt number to python custom grader Aug 3, 2018

expose attempts to python custom grader

ddd8d41

pdpinch force-pushed the pdpinch/expose-attempts branch from e7bfb47 to ddd8d41 Compare August 27, 2018 15:50

schenedx reviewed Aug 27, 2018

View reviewed changes

pdpinch and others added 3 commits August 30, 2018 15:57

clarify comments

3763d26

Handle situation where attempts have been reset to 0 and problem is r…

28e64d9

…egraded; Update comments

Remember to add changes before committing...

9981b15

schenedx approved these changes Sep 10, 2018

View reviewed changes

schenedx merged commit af3e5ba into openedx:master Sep 10, 2018

jolyonb mentioned this pull request Sep 16, 2018

Implement options for grading based on attempt number mitodl/mitx-grading-library#137

Closed

jolyonb mentioned this pull request Jul 6, 2019

Setting up attempt-based grading mitodl/mitx-grading-library#248

Merged

pdpinch deleted the pdpinch/expose-attempts branch April 9, 2020 11:46

openedx-webhooks added merged and removed awaiting prioritization labels Jan 22, 2021

Uh oh!

Conversation

jolyonb commented May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented May 7, 2018

Uh oh!

mduboseedx commented May 8, 2018

Uh oh!

jolyonb commented May 8, 2018

Uh oh!

mduboseedx commented May 8, 2018

Uh oh!

pdpinch commented Jul 1, 2018

Uh oh!

pdpinch commented Jul 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pdpinch commented Jul 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jolyonb commented Jul 3, 2018

Uh oh!

jolyonb commented Aug 3, 2018

Uh oh!

pdpinch commented Aug 6, 2018

Uh oh!

mduboseedx commented Aug 6, 2018

Uh oh!

jolyonb commented Aug 20, 2018

Uh oh!

mduboseedx commented Aug 21, 2018

Uh oh!

pdpinch commented Aug 27, 2018

Uh oh!

schenedx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edx-status-bot commented Aug 30, 2018

Uh oh!

jolyonb commented Aug 30, 2018

Uh oh!

mduboseedx commented Sep 10, 2018

Uh oh!

schenedx left a comment

Choose a reason for hiding this comment

Uh oh!

openedx-webhooks commented Sep 10, 2018

Uh oh!

edx-pipeline-bot commented Sep 10, 2018

Uh oh!

edx-pipeline-bot commented Sep 10, 2018

Uh oh!

jolyonb commented Sep 10, 2018

Uh oh!

edx-pipeline-bot commented Sep 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jolyonb commented May 7, 2018 •

edited

Loading