OctaveCodeQuestion: Octave/MATLAB support in RELATE

[davis68]

This PR contains a tested and working OctaveCodeQuestion page type and the appropriate Dockerfile, inducer/runoc.

As of the initial set of commits, there is not any documentation or unit testing included.

This is in response to #631.

[davis68]

Looks like checks are failing since the testing container doesn't know where/what the inducer/runoc image is.

[davis68]

@inducer I want to bump this and see if there's a chance of reviewing and updating with it before April 15. Thanks.

[inducer]

Sorry for the long silence here. We should be able to get this in. If worst comes to worst, I'll run the UIUC instance on this branch for a bit.

[inducer]

Looks like checks are failing since the testing container doesn't know where/what the inducer/runoc image is.

I don't think that's the case. The tests don't rely on Docker being present/usable as far as I know. But making sure the CIs know about Octave might require a bit of work. (which I might be able to help with)

[davis68]

Got it. Don't worry about the timeline, then. I have a fallback for this semester's exercises.

In general, we should decide on the best superclass model for new code question types and move from there.

[inducer]

Got it. Don't worry about the timeline, then.

You sure? I'm pretty sure we can still get it by then.

[davis68]

Unsurprisingly, a lot of the duplication is because most of the code is either Python-specific or is named in such a way that it indicates Python specificity. I created octave/oc equivalents. Instead, what we should do is superclass a CodeQuestion that PythonCodeQuestion and OctaveCodeQuestion derive from. CodeQuestion should require implementation of:

• request_run
• request_run_with_retries
• allowed_attrs
• __init__, body, grade, correct_answer, etc.

I will diff these and see what a superclass should look like. Right now PythonCodeQuestion derives from PageBaseWithTitle and PageBaseWithValue. Should CodeQuestion derive from these instead or be an additional class?

There are external supporting files like code_runpy_backend.py which will persist unless we rework that entire structure.

[davis68]

About 20 lines of code are actually different, so it's fairly minor. Is there any reason to retain RUNPY_PORT separate from RUNOC_PORT? Else I'll merge them into RUN_PORT at 9941.

[inducer]

Unifying them seems like the right idea. CODE_QUESTION_CONTAINER_PORT maybe?

[davis68]

Stylistic question:

Do you prefer

1. A comprehensive CodeQuestion superclass ("interface") which changes file types, Docker images, etc. based on the language_mode attribute. In this case, PythonCodeQuestion and OctaveCodeQuestion just call CodeQuestion.__init__ with a particular self.language_mode.

    def __init__(self, vctx, location, page_desc):
        super(PythonCodeQuestion, self).__init__(self, vctx, location, page_desc, language_mode='python')

if language_mode == 'python':
        RELATE_DOCKER_IMAGE = settings.RELATE_DOCKER_RUNPY_IMAGE
        command_path = "/opt/runpy/runpy"
    elif language_mode == 'octave':
        RELATE_DOCKER_IMAGE = settings.RELATE_DOCKER_RUNOC_IMAGE
        command_path = "/opt/runoc/runoc"

2. A minimalist CodeQuestion superclass. In this case, PCQ and OCQ re-implement a lot of common code but we avoid if/elif blocks.

[davis68]

I lean towards the former since it reduces code duplication the most. Those if/elif blocks are just ugly though.

[inducer]

Honestly, neither. All shared logic should definitely reside in the superclass. (I don't deal well with duplicated code.) Variant data (such as image or command names) should be queryable by a (virtual) method call or a property. E.g.

class CodeQuestionBase(...):
    def run_stuff(self, ...):
        stuff(self.container_image)

class PythonCodeQuestion(...):
    @property
    def container_image(self):
        return settings.RUNPY....

[inducer]

I.e. use inheritance-based dispatch whenever you're tempted to say insert an if.

[davis68]

OK, that was pretty straightforward to implement. The hard thing at this point is how to let run_code know what kind of code it's running.

Right now this is determined at the level of importing code:

        from .code_runpy_backend import substitute_correct_code_into_test_code
        return substitute_correct_code_into_test_code(test_code, correct_code)

So code_runpy_backend doesn't know about a language_mode or any of the code question internals. code_runoc_backend just provides a different run_code. It's not apparent to me right now whether this can be cleanly merged or if we'll have to maintain two separate but similar code_run??_backend.py scripts. I'll chew on it tonight—maybe there could be some magic via importlib...

And this affects the Docker image build as well.

[davis68]

Wait, I've got it. I don't think any of the code_run??_backend internals matter to RELATE, only that they exist. So we can unify it as an interface for RELATE, but copy in a different source file for the two different Docker images. I'll check into this.

[codecov[bot]]

Codecov Report

Merging #633 into master will decrease coverage by 13.57%. The diff coverage is 10.37%.

@@             Coverage Diff             @@
##           master     #633       +/-   ##
===========================================
- Coverage   96.93%   83.35%   -13.58%     
===========================================
  Files          45       46        +1     
  Lines       11096    11501      +405     
  Branches     2062     2143       +81     
===========================================
- Hits        10756     9587     -1169     
- Misses        292     1700     +1408     
- Partials       48      214      +166

Impacted Files	Coverage Δ
course/page/code_runoc_backend.py	0% <0%> (ø)
course/page/__init__.py	100% <100%> (ø)	:arrow_up:
course/page/code.py	17.54% <12.61%> (-81.21%)	:arrow_down:
course/tasks.py	19.71% <0%> (-80.29%)	:arrow_down:
accounts/admin.py	55.1% <0%> (-44.9%)	:arrow_down:
course/grading.py	63.93% <0%> (-36.07%)	:arrow_down:
course/admin.py	65.72% <0%> (-32.49%)	:arrow_down:
course/analytics.py	71.65% <0%> (-27.56%)	:arrow_down:
accounts/utils.py	79.48% <0%> (-20.52%)	:arrow_down:
... and 20 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d4e1f36...6f8980c. Read the comment docs.

[codecov[bot]]

Codecov Report

Merging #633 into master will decrease coverage by 57.55%. The diff coverage is 44%.

@@             Coverage Diff             @@
##           master     #633       +/-   ##
===========================================
- Coverage   96.93%   39.37%   -57.56%     
===========================================
  Files          45       45               
  Lines       11096    11120       +24     
  Branches     2062     2062               
===========================================
- Hits        10756     4379     -6377     
- Misses        292     6261     +5969     
- Partials       48      480      +432

Impacted Files	Coverage Δ
course/page/base.py	45.64% <0%> (-54.36%)	:arrow_down:
course/page/__init__.py	100% <100%> (ø)	:arrow_up:
course/page/code.py	17.7% <45.65%> (-81.05%)	:arrow_down:
course/tasks.py	0% <0%> (-100%)	:arrow_down:
course/grading.py	10.38% <0%> (-89.62%)	:arrow_down:
course/grades.py	10.66% <0%> (-86.49%)	:arrow_down:
course/sandbox.py	15.6% <0%> (-84.4%)	:arrow_down:
course/views.py	17.84% <0%> (-82.16%)	:arrow_down:
course/calendar.py	20.17% <0%> (-79.83%)	:arrow_down:
course/enrollment.py	19.2% <0%> (-79.61%)	:arrow_down:
... and 27 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d4e1f36...8c2d4f0. Read the comment docs.

[davis68]

This version works locally for me.

This has entailed some modest systemic changes:

• CodeQuestion, most obviously. PythonCodeQuestion and OctaveCodeQuestion are mostly docstrings, with a couple of properties attached to specify the container and behavior.. This PythonCodeQuestion should be drop-in everywhere.
• the user inside of the docker images is runcode, and the scripts are similarly named
• the scripts for Python and Octave are different but present the same interface and are built into the Docker images
• I am referring to davis68/relate-octave for the Octave image right now
• Some of the variables are renamed to indicate that they are not Python-exclusive. I have renamed them everywhere they are used except in the tests. Notably, request_run and request_run_with_retries.

This gives RELATE a basis for future code question types that range beyond Python as long as there is a Python interface.

At this point, however, there isn't any testing language for OctaveCodeQuestion, nor is there an OctaveCodeQuestionWithHumanFeedback. I will work on these tests next.

[davis68]

I can revert the renaming of the in-container user name. It's currently runcode because runpy is Python-specific. If you prefer not to change the inducer/relate-runpy-amd64 image at all, then I'll accommodate that here.

[davis68]

I've replicated the test_code.py tests for Python in Octave now. The tests are now mainly failing on the AssertionError:

AssertionError: 'localhost' != '192.168.1.100'

[davis68]

(I'm writing here to document; I'll let you know when it's ready for a more in-depth review.)

[davis68]

@inducer the outstanding failed tests from AppVeyor are all of the form

======================================================================
FAIL: test_image_none (tests.test_pages.test_code.RequestOctaveRunWithRetriesTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\python35-x64\lib\site-packages\django\test\utils.py", line 371, in inner
    return func(*args, **kwargs)
  File "C:\projects\relate\tests\test_pages\test_code.py", line 1602, in test_image_none
    self.assertEqual(mock_create_ctn.call_count, 1)
AssertionError: 0 != 1
======================================================================
FAIL: test_docker_container_ping_failure (tests.test_pages.test_code.RequestPythonRunWithRetriesTest) (case='Docker ping timeout with BadStatusLine Error')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\relate\tests\test_pages\test_code.py", line 1373, in test_docker_container_ping_failure
    self.assertEqual(res["exec_host"], fake_host_ip)
AssertionError: 'localhost' != '192.168.1.100'
- localhost
+ 192.168.1.100
======================================================================

These don't (all) originate in code I've changed. Barring these, I think this is ready for review.

[davis68]

What is the reasoning behind fake_host_ip = "192.168.1.100"?

[inducer]

Trying to merge master into this gives me pretty puzzling results, as if this branch were trying to repeat changes from #667. Not even merging e77748c65007179309961f884af3e71148e624af (the merge commit for #667) into this is conflict-free. @davis68, could you take a look?

[inducer]

@davis68, I saw your comment on #631. So the status of this PR is that it has been gutted to produce #674 and #667, and the remaining Octave changes are waiting to be put into another, separate PR?

[davis68]

Yes, I pulled it apart. The remaining Octave changes are mostly done except for reproducing and satisfying the appropriate tests for Octave instead of Python.

[davis68]

Per my comment from last year:

At this point, however, there isn't any testing language for OctaveCodeQuestion, nor is there an OctaveCodeQuestionWithHumanFeedback. I will work on these tests next.