OctaveCodeQuestion: Octave/MATLAB support in RELATE #633

davis68 · 2018-12-30T22:14:07Z

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.

Catch up.

davis68 · 2018-12-31T14:14:33Z

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

davis68 · 2019-03-23T23:41:44Z

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

inducer · 2019-03-24T18:11:13Z

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 · 2019-03-24T18:16:53Z

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)

inducer · 2019-03-24T18:14:53Z

local_settings_example.py

@@ -343,6 +343,7 @@
 # A string containing the image ID of the docker image to be used to run
 # student Python code. Docker should download the image on first run.


Adapt comment.

inducer · 2019-03-24T18:15:30Z

course/page/code.py

@@ -1190,4 +1190,945 @@ def grade(self, page_context, page_data, answer_data, grade_data):

 # }}}



Throughout this file, there is far too much code duplication for my liking. For example:

I would much prefer if both code questions inherited from a common base class.

Why are there two code forms?

Why are there two copies of request_*_run?

To explain the rationale:

Duplicated code is very difficult to review: The changes are not obvious.

Duplicated code is even harder to maintain---a cost that I'm reluctant to shoulder.

davis68 · 2019-03-25T20:53:27Z

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 · 2019-03-25T21:56:28Z

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

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

davis68 · 2019-03-26T15:07:24Z

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 · 2019-03-26T15:31:48Z

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 · 2019-03-26T20:03:12Z

Unifying them seems like the right idea. CODE_QUESTION_CONTAINER_PORT maybe?

davis68 · 2019-04-02T19:56:42Z

Stylistic question:

Do you prefer

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"

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

davis68 · 2019-04-02T20:00:07Z

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

inducer · 2019-04-02T20:08:52Z

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 · 2019-04-02T20:09:26Z

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

davis68 · 2019-04-02T20:43:06Z

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 · 2019-04-02T20:52:46Z

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 · 2019-04-02T21:56:29Z

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%> (ø)`	⬆️
course/page/code.py	`17.54% <12.61%> (-81.21%)`	⬇️
course/tasks.py	`19.71% <0%> (-80.29%)`	⬇️
accounts/admin.py	`55.1% <0%> (-44.9%)`	⬇️
course/grading.py	`63.93% <0%> (-36.07%)`	⬇️
course/admin.py	`65.72% <0%> (-32.49%)`	⬇️
course/analytics.py	`71.65% <0%> (-27.56%)`	⬇️
accounts/utils.py	`79.48% <0%> (-20.52%)`	⬇️
... 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 · 2019-04-02T21:56:29Z

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%)`	⬇️
course/page/__init__.py	`100% <100%> (ø)`	⬆️
course/page/code.py	`17.7% <45.65%> (-81.05%)`	⬇️
course/tasks.py	`0% <0%> (-100%)`	⬇️
course/grading.py	`10.38% <0%> (-89.62%)`	⬇️
course/grades.py	`10.66% <0%> (-86.49%)`	⬇️
course/sandbox.py	`15.6% <0%> (-84.4%)`	⬇️
course/views.py	`17.84% <0%> (-82.16%)`	⬇️
course/calendar.py	`20.17% <0%> (-79.83%)`	⬇️
course/enrollment.py	`19.2% <0%> (-79.61%)`	⬇️
... 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 · 2019-04-03T02:46:53Z

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 · 2019-04-03T02:48:47Z

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 · 2019-04-09T14:31:43Z

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 · 2019-04-09T14:43:31Z

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

davis68 · 2019-04-10T23:44:43Z

@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 · 2019-04-12T20:34:50Z

What is the reasoning behind fake_host_ip = "192.168.1.100"?

inducer · 2020-07-28T19:50:39Z

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 e77748c (the merge commit for #667) into this is conflict-free. @davis68, could you take a look?

inducer · 2020-07-28T20:55:08Z

@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 · 2020-07-28T21:34:51Z

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 · 2020-07-28T21:36:55Z

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.

davis68 added 5 commits December 12, 2018 13:58

Merge pull request #4 from inducer/master

4358682

Catch up.

Post basic Octave support.

c0192c8

Use two steps to handle copying code over to Docker image.

50ca0d1

Flatten Docker image back into one.

0e8768d

Include Docker image by name. It needs to be built first.

3491cd3

inducer reviewed Mar 24, 2019

View reviewed changes

First stab at a CodeQuestion interface. UNTESTED but pushed for backup

6f8980c

davis68 added 3 commits April 2, 2019 19:03

Fixed the CodeQuestion blocks.

146c0aa

Everything seems to be working properly now.

3700301

Post davis68 version of docker image.

ef05a70

Post changes to tests.

8bfe887

davis68 added 4 commits April 2, 2019 22:24

Post changes to tests.

a4cedef

Post tests.

1e6051b

Fix some imports from older version of code.

cd7cfca

Fix some imports from older version of code.

c477cd0

Turn off debugging.

8c2d4f0

Base automatically changed from master to main March 8, 2021 02:15

davis68 mentioned this pull request Apr 7, 2021

OctaveCodeQuestion redux #786

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OctaveCodeQuestion: Octave/MATLAB support in RELATE #633

OctaveCodeQuestion: Octave/MATLAB support in RELATE #633

davis68 commented Dec 30, 2018

davis68 commented Dec 31, 2018

davis68 commented Mar 23, 2019

inducer commented Mar 24, 2019

inducer commented Mar 24, 2019

inducer Mar 24, 2019

inducer Mar 24, 2019

inducer Mar 24, 2019

davis68 commented Mar 25, 2019

inducer commented Mar 25, 2019

davis68 commented Mar 26, 2019

davis68 commented Mar 26, 2019

inducer commented Mar 26, 2019

davis68 commented Apr 2, 2019 •

edited

davis68 commented Apr 2, 2019

inducer commented Apr 2, 2019

inducer commented Apr 2, 2019

davis68 commented Apr 2, 2019 •

edited

davis68 commented Apr 2, 2019 •

edited

codecov bot commented Apr 2, 2019

codecov bot commented Apr 2, 2019 •

edited

davis68 commented Apr 3, 2019

davis68 commented Apr 3, 2019

davis68 commented Apr 9, 2019

davis68 commented Apr 9, 2019

davis68 commented Apr 10, 2019

davis68 commented Apr 12, 2019

inducer commented Jul 28, 2020

inducer commented Jul 28, 2020

davis68 commented Jul 28, 2020

davis68 commented Jul 28, 2020

		@@ -343,6 +343,7 @@
		# A string containing the image ID of the docker image to be used to run
		# student Python code. Docker should download the image on first run.

		@@ -1190,4 +1190,945 @@ def grade(self, page_context, page_data, answer_data, grade_data):

		# }}}

OctaveCodeQuestion: Octave/MATLAB support in RELATE #633

Are you sure you want to change the base?

OctaveCodeQuestion: Octave/MATLAB support in RELATE #633

Conversation

davis68 commented Dec 30, 2018

davis68 commented Dec 31, 2018

davis68 commented Mar 23, 2019

inducer commented Mar 24, 2019

inducer commented Mar 24, 2019

inducer Mar 24, 2019

Choose a reason for hiding this comment

inducer Mar 24, 2019

Choose a reason for hiding this comment

inducer Mar 24, 2019

Choose a reason for hiding this comment

davis68 commented Mar 25, 2019

inducer commented Mar 25, 2019

davis68 commented Mar 26, 2019

davis68 commented Mar 26, 2019

inducer commented Mar 26, 2019

davis68 commented Apr 2, 2019 • edited

davis68 commented Apr 2, 2019

inducer commented Apr 2, 2019

inducer commented Apr 2, 2019

davis68 commented Apr 2, 2019 • edited

davis68 commented Apr 2, 2019 • edited

codecov bot commented Apr 2, 2019

Codecov Report

codecov bot commented Apr 2, 2019 • edited

Codecov Report

davis68 commented Apr 3, 2019

davis68 commented Apr 3, 2019

davis68 commented Apr 9, 2019

davis68 commented Apr 9, 2019

davis68 commented Apr 10, 2019

davis68 commented Apr 12, 2019

inducer commented Jul 28, 2020

inducer commented Jul 28, 2020

davis68 commented Jul 28, 2020

davis68 commented Jul 28, 2020

davis68 commented Apr 2, 2019 •

edited

davis68 commented Apr 2, 2019 •

edited

davis68 commented Apr 2, 2019 •

edited

codecov bot commented Apr 2, 2019 •

edited