-
Notifications
You must be signed in to change notification settings - Fork 115
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
OctaveCodeQuestion: Octave/MATLAB support in RELATE #633
base: main
Are you sure you want to change the base?
Conversation
Looks like checks are failing since the testing container doesn't know where/what the |
@inducer I want to bump this and see if there's a chance of reviewing and updating with it before April 15. Thanks. |
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. |
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) |
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adapt comment.
@@ -1190,4 +1190,945 @@ def grade(self, page_context, page_data, answer_data, grade_data): | |||
|
|||
# }}} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
You sure? I'm pretty sure we can still get it by then. |
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
I will There are external supporting files like |
About 20 lines of code are actually different, so it's fairly minor. Is there any reason to retain |
Unifying them seems like the right idea. |
Stylistic question: Do you prefer
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"
|
I lean towards the former since it reduces code duplication the most. Those if/elif blocks are just ugly though. |
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.... |
I.e. use inheritance-based dispatch whenever you're tempted to say insert an |
OK, that was pretty straightforward to implement. The hard thing at this point is how to let 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 And this affects the Docker image build as well. |
Wait, I've got it. I don't think any of the |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
This version works locally for me. This has entailed some modest systemic changes:
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 |
I can revert the renaming of the in-container user name. It's currently |
I've replicated the
|
(I'm writing here to document; I'll let you know when it's ready for a more in-depth review.) |
@inducer the outstanding failed tests from AppVeyor are all of the form
These don't (all) originate in code I've changed. Barring these, I think this is ready for review. |
What is the reasoning behind |
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. |
Per my comment from last year:
|
This PR contains a tested and working
OctaveCodeQuestion
page type and the appropriateDockerfile
,inducer/runoc
.As of the initial set of commits, there is not any documentation or unit testing included.
This is in response to #631.