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
Run ProgrammingEvaluation in service #2308
Run ProgrammingEvaluation in service #2308
Conversation
ddbecfb
to
a2901fb
Compare
end | ||
|
||
stream.close | ||
result |
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.
Inconsistent indentation detected.
result[block.stream] << block.bytes | ||
end | ||
|
||
stream.close |
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.
Inconsistent indentation detected.
result = [''.dup, ''.dup, ''.dup] | ||
stream = StringIO.new(string) | ||
|
||
while (block = parse_docker_stream_read_block(stream)) |
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.
Inconsistent indentation detected.
# @return [Array<(String, String, String)>] The stdin, stdout, and stderr output. | ||
def parse_docker_stream(string) | ||
result = [''.dup, ''.dup, ''.dup] | ||
stream = StringIO.new(string) |
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.
Inconsistent indentation detected.
# @param [String] string The input stream to parse. | ||
# @return [Array<(String, String, String)>] The stdin, stdout, and stderr output. | ||
def parse_docker_stream(string) | ||
result = [''.dup, ''.dup, ''.dup] |
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.
Tab detected.
Use 2 (not 0) spaces for indentation.
|
||
zip_entry_stream = entry.get_input_stream | ||
new_entry_name = prefix ? File.join(prefix, entry.name) : entry.name | ||
tar_file.add_file(new_entry_name, 0664) do |tar_entry_stream| |
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.
Use 0o for octal literals.
tar_file_stream | ||
end | ||
|
||
# Copies every entry from the zip archive to the tar archive, adding the optional prefix to the |
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.
Tab detected.
Incorrect indentation detected (column 1 instead of 2).
end | ||
end | ||
|
||
# Converts the zip package into a tar package for the container. |
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.
Tab detected.
Incorrect indentation detected (column 1 instead of 2).
def copy_package(container) | ||
tar = tar_package(@package) | ||
container.archive_in_stream(HOME_PATH) do | ||
tar.read(Excon.defaults[:chunk_size]).to_s |
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.
Tab detected.
Use 2 (not -1) spaces for indentation.
# @param [Docker::Container] container The container to copy the package into. | ||
def copy_package(container) | ||
tar = tar_package(@package) | ||
container.archive_in_stream(HOME_PATH) do |
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.
Inconsistent indentation detected.
a2901fb
to
0453437
Compare
result | ||
end | ||
|
||
# Copies the contents of the package to the container. |
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.
Tab detected.
Incorrect indentation detected (column 1 instead of 2).
result.push("-m#{@evaluation.memory_limit * MEMORY_LIMIT_RATIO}") if @evaluation.memory_limit | ||
|
||
result | ||
end |
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.
end at 155, 2 is not aligned with def at 149, 1.
end | ||
|
||
def container_arguments | ||
result = [] |
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.
Use 2 (not 3) spaces for indentation.
CoursemologyDockerContainer.create(image_identifier, argv: container_arguments) | ||
end | ||
|
||
def container_arguments |
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.
Tab detected.
Inconsistent indentation detected.
|
||
def create_container(image) | ||
image_identifier = "coursemology/evaluator-image-#{image}" | ||
CoursemologyDockerContainer.create(image_identifier, argv: container_arguments) |
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.
Inconsistent indentation detected.
@@ -123,4 +140,104 @@ def wait_for_evaluation(evaluation) | |||
|
|||
raise Timeout::Error if wait_result.nil? | |||
end | |||
|
|||
def create_container(image) | |||
image_identifier = "coursemology/evaluator-image-#{image}" |
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.
Tab detected.
Use 2 (not 0) spaces for indentation.
ensure | ||
evaluation.destroy! if evaluation | ||
@evaluation.destroy! if @evaluation | ||
container.delete if container |
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.
Use safe navigation (&.) instead of checking if an object exists before calling the method.
ensure | ||
evaluation.destroy! if evaluation | ||
@evaluation.destroy! if @evaluation |
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.
Use safe navigation (&.) instead of checking if an object exists before calling the method.
@@ -5,6 +5,18 @@ class Course::Assessment::ProgrammingEvaluationService | |||
DEFAULT_TIMEOUT = Course::Assessment::ProgrammingEvaluation::TIMEOUT | |||
CPU_TIMEOUT = Course::Assessment::ProgrammingEvaluation::CPU_TIMEOUT | |||
|
|||
# The path to the Coursemology user home directory. | |||
HOME_PATH = '/home/coursemology'.freeze |
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.
Freezing immutable objects is pointless.
@@ -5,6 +5,18 @@ class Course::Assessment::ProgrammingEvaluationService | |||
DEFAULT_TIMEOUT = Course::Assessment::ProgrammingEvaluation::TIMEOUT | |||
CPU_TIMEOUT = Course::Assessment::ProgrammingEvaluation::CPU_TIMEOUT | |||
|
|||
# The path to the Coursemology user home directory. |
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.
Tab detected.
Incorrect indentation detected (column 1 instead of 2).
6d4b6c6
to
bd4b649
Compare
thread.abort_on_exception = true | ||
def evaluate_in_container | ||
attributes = FactoryGirl.attributes_for(:course_assessment_programming_evaluation, :completed). | ||
slice(:stdout, :stderr, :test_report, :exit_code) |
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.
Align slice with FactoryGirl.attributes_for(:course_assessment_programming_evaluation, :completed). on line 6.
# | ||
# @return [Array<(String, String, String, Integer)>] The stdout, stderr, test report and exit | ||
# code. | ||
def get_evaluation_result |
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.
Do not prefix reader method names with get_.
retry | ||
end | ||
|
||
|
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.
Extra blank line detected.
# frozen_string_literal: true | ||
class CoursemologyDockerContainer < Docker::Container | ||
# The path to the Coursemology user home directory. | ||
HOME_PATH = '/home/coursemology'.freeze |
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.
Freezing immutable objects is pointless.
@@ -0,0 +1,211 @@ | |||
# frozen_string_literal: true | |||
class CoursemologyDockerContainer < Docker::Container |
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.
Class has too many lines. [118/100]
bd4b649
to
867b11e
Compare
@@ -0,0 +1,207 @@ | |||
# frozen_string_literal: true | |||
class CoursemologyDockerContainer < Docker::Container |
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.
Class has too many lines. [116/100]
51edf0d
to
e6799dd
Compare
def evaluate_in_container | ||
attributes = FactoryGirl.attributes_for(:course_assessment_programming_evaluation, :completed). | ||
slice(:stdout, :stderr, :test_report, :exit_code) | ||
sleep(0.2) # For timeout testing |
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.
Unnecessary spacing detected.
@@ -94,30 +94,10 @@ | |||
expect do | |||
subject.execute(course, Coursemology::Polyglot::Language::Python::Python2Point7.instance, | |||
64, 5.seconds, File.join(Rails.root, 'spec', 'fixtures', 'course', | |||
'programming_question_template.zip'), 0.seconds) | |||
'programming_question_template.zip'), | |||
0.1.seconds) |
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.
Align the parameters of a method call if they span more than one line.
ActiveSupport::Notifications.instrument('create.docker.evaluator.coursemology', | ||
image: image) do |payload| | ||
options = { 'Image' => image } | ||
options['Cmd'] = argv if argv && !argv.empty? |
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.
Use argv.present? instead of argv && !argv.empty?.
# frozen_string_literal: true | ||
class CoursemologyDockerContainer < Docker::Container | ||
# The path to the Coursemology user home directory. | ||
HOME_PATH = '/home/coursemology'.freeze |
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.
Do not freeze immutable objects, as freezing them has no effect.
@@ -0,0 +1,210 @@ | |||
# frozen_string_literal: true | |||
class CoursemologyDockerContainer < Docker::Container |
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.
Add an empty line after magic comments.
Class has too many lines. [118/100]
def evaluate_in_container | ||
attributes = FactoryGirl.attributes_for(:course_assessment_programming_evaluation, :completed). | ||
slice(:stdout, :stderr, :test_report, :exit_code) | ||
sleep(0.2) # For timeout testing |
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.
Unnecessary spacing detected.
@@ -94,30 +94,10 @@ | |||
expect do | |||
subject.execute(course, Coursemology::Polyglot::Language::Python::Python2Point7.instance, | |||
64, 5.seconds, File.join(Rails.root, 'spec', 'fixtures', 'course', | |||
'programming_question_template.zip'), 0.seconds) | |||
'programming_question_template.zip'), | |||
0.1.seconds) |
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.
Align the parameters of a method call if they span more than one line.
ActiveSupport::Notifications.instrument('create.docker.evaluator.coursemology', | ||
image: image) do |payload| | ||
options = { 'Image' => image } | ||
options['Cmd'] = argv if argv && !argv.empty? |
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.
Use argv.present? instead of argv && !argv.empty?.
# frozen_string_literal: true | ||
class CoursemologyDockerContainer < Docker::Container | ||
# The path to the Coursemology user home directory. | ||
HOME_PATH = '/home/coursemology'.freeze |
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.
Do not freeze immutable objects, as freezing them has no effect.
@@ -0,0 +1,210 @@ | |||
# frozen_string_literal: true | |||
class CoursemologyDockerContainer < Docker::Container |
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.
Add an empty line after magic comments.
Class has too many lines. [118/100]
418aebb
to
410f7da
Compare
subject.execute(course, Coursemology::Polyglot::Language::Python::Python2Point7.instance, | ||
64, 5.seconds, File.join(Rails.root, 'spec', 'fixtures', 'course', | ||
'programming_question_template.zip'), 0.seconds) | ||
'programming_question_template.zip'), | ||
0.1.seconds) |
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.
Align the parameters of a method call if they span more than one line.
410f7da
to
e73821a
Compare
class CoursemologyDockerContainer < Docker::Container | ||
|
||
# The path to the Coursemology user home directory. | ||
HOME_PATH = '/home/coursemology'.freeze |
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.
Do not freeze immutable objects, as freezing them has no effect.
@@ -0,0 +1,211 @@ | |||
# frozen_string_literal: true | |||
class CoursemologyDockerContainer < Docker::Container | |||
|
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.
Extra empty line detected at class body beginning.
e73821a
to
dbde6f9
Compare
@@ -0,0 +1,211 @@ | |||
# frozen_string_literal: true | |||
|
|||
class CoursemologyDockerContainer < Docker::Container |
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.
Class has too many lines. [118/100]
@@ -0,0 +1,211 @@ | |||
# frozen_string_literal: true | |||
|
|||
class CoursemologyDockerContainer < Docker::Container |
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.
Class has too many lines. [118/100]
@@ -0,0 +1,211 @@ | |||
# frozen_string_literal: true | |||
|
|||
class CoursemologyDockerContainer < Docker::Container |
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.
Class has too many lines. [118/100]
let(:time_limit) { nil } | ||
let(:service_instance) do | ||
subject.new(course, Coursemology::Polyglot::Language::Python::Python2Point7.instance, | ||
memory_limit, time_limit, File.join(Rails.root, 'spec', 'fixtures', 'course', |
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.
Please use Rails.root.join('path', 'to') instead.
65f2b5d
to
0337c89
Compare
new(course, Coursemology::Polyglot::Language::Python::Python2Point7.instance, 64, | ||
5.seconds, File.join(Rails.root, 'spec', 'fixtures', 'course', | ||
'programming_question_template.zip'), 5.seconds) | ||
describe '#create_container' do |
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.
Block has too many lines. [26/25]
end | ||
end | ||
|
||
describe '#extract_test_report' do |
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.
Block has too many lines. [27/25]
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.
won't fix
after { subject.send(:delete) } | ||
|
||
it 'does not expose raw Docker Attach Protocol in the output' do | ||
stdout, stderr, _, _ = subject.send(:evaluation_result) |
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.
Do not use trailing _s in parallel assignment. Prefer stdout, stderr, = subject.send(:evaluation_result).
it 'returns the exit code of the container' do | ||
expect(subject.wait).to eq(subject.exit_code) | ||
end | ||
end |
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.
end at 30, 2 is not aligned with describe '#wait' do at 11, 1.
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.
This is outdated.
end | ||
subject { CoursemologyDockerContainer.create(image) } | ||
|
||
describe '#wait' do |
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.
Tab detected.
Inconsistent indentation detected.
# frozen_string_literal: true | ||
require 'rails_helper' | ||
|
||
RSpec.describe CoursemologyDockerContainer do |
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.
Block has too many lines. [111/25]
@@ -0,0 +1,140 @@ | |||
# frozen_string_literal: true | |||
require 'rails_helper' |
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.
Add an empty line after magic comments.
0337c89
to
db59a4c
Compare
|
||
class CoursemologyDockerContainer < Docker::Container | ||
# The path to the Coursemology user home directory. | ||
HOME_PATH = '/home/coursemology'.freeze |
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.
So evaluator will assume that there's a coursemology user ?
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.
Yes. This setup is from the docker file
bc734f8
to
2b176f6
Compare
Provides abstractions to work with Docker containers.
Previously, the ProgrammingEvaluationService created evaluations and waited for external evaluators to run the evaluation and update its state. To elimimate the polling time of these external evaluators, move actual evaluation using docker containers into the service so the worker can run it immediately. No longer create ProgrammingEvaluation objects. Use Ruby's timeout on the execute method instead of the old method of using the custom AR extension for the ProgrammingEvaluation object.
Return the expected array of values during testing instead of spinning up a Docker container for actual evaluation. Add a short sleep to facilitate timeout testing. Remove `wait_for_evaluation` stub. Fix ProgrammingEvaluationService specs. Use a non-zero timeout as Ruby timeout treats 0 as infinite. Remove create_evaluation tests.
Adapted from corresponding specs in evaluator-slave.
Adapted from corresponding specs in evaluator-slave. Specify use of Docker to Travis. Install nodejs 6 as VM based trusty uses an old incompatible version.
* The first component can be random depends on the order of the loading
2b176f6
to
cd37f09
Compare
Evaluate packages directly in ProgrammingEvaluationService instead of waiting for an external evaluator.
This will save the polling time, but worker servers will need to have Docker installed.
DO NOT MERGE YET.
For initial feedback, some refactoring and additional tests will be necessary.