From 002c806c313b2b2291db9aeae75d793dbe4ea3b6 Mon Sep 17 00:00:00 2001 From: Chris Pyles Date: Sat, 15 Jul 2023 21:21:04 -0700 Subject: [PATCH 1/5] add config overrides to otter grade --- otter/cli.py | 1 + otter/grade/__init__.py | 8 ++++++++ otter/grade/containers.py | 37 ++++++++++++++++++++++++++++++------- 3 files changed, 39 insertions(+), 7 deletions(-) diff --git a/otter/cli.py b/otter/cli.py index 779daec6a..2e9cfb41f 100644 --- a/otter/cli.py +++ b/otter/cli.py @@ -137,6 +137,7 @@ def generate_cli(*args, **kwargs): @click.option("--timeout", type=click.INT, help="Submission execution timeout in seconds") @click.option("--no-network", is_flag=True, help="Disable networking in the containers") @click.option("--no-kill", is_flag=True, help="Do not kill containers after grading") +@click.option("--debug", is_flag=True, help="Run in debug mode (without ignoring errors thrown during execution)") @click.option("--prune", is_flag=True, help="Prune all of Otter's grading images") @click.option("-f", "--force", is_flag=True, help="Force action (don't ask for confirmation)") def grade_cli(*args, **kwargs): diff --git a/otter/grade/__init__.py b/otter/grade/__init__.py index 143ad3150..44b7e7a97 100644 --- a/otter/grade/__init__.py +++ b/otter/grade/__init__.py @@ -9,6 +9,7 @@ from .containers import launch_containers from .utils import merge_csv, prune_images +from ..run.run_autograder.autograder_config import AutograderConfig from ..utils import assert_path_exists, loggers @@ -31,6 +32,7 @@ def main( force: bool = False, timeout: bool = None, no_network: bool = False, + debug: bool = False, ): """ Run Otter Grade. @@ -58,6 +60,7 @@ def main( force (``bool``): whether to force-prune the images (do not ask for confirmation) timeout (``int``): an execution timeout in seconds for each container no_network (``bool``): whether to disable networking in the containers + debug (``bool``): whether to run autograding in debug mode Returns: ``float | None``: the percentage scored by that submission if a single file was graded @@ -114,6 +117,11 @@ def main( pdf_dir = pdf_dir, timeout = timeout, network = not no_network, + config = AutograderConfig({ + "zips": ext == "zip", + "pdf": pdfs, + "debug": debug, + }), ) LOGGER.info("Combining grades and saving") diff --git a/otter/grade/containers.py b/otter/grade/containers.py index 7be84ac7a..ef702f5bb 100644 --- a/otter/grade/containers.py +++ b/otter/grade/containers.py @@ -1,7 +1,9 @@ """Docker container management for Otter Grade""" +import json import os import pandas as pd +import pathlib import pkg_resources import shutil import tempfile @@ -14,6 +16,7 @@ from .utils import OTTER_DOCKER_IMAGE_NAME +from ..run.run_autograder.autograder_config import AutograderConfig from ..utils import loggers @@ -21,7 +24,7 @@ LOGGER = loggers.get_logger(__name__) -def build_image(ag_zip_path: str, base_image: str, tag: str): +def build_image(ag_zip_path: str, base_image: str, tag: str, config: AutograderConfig): """ Creates a grading image based on the autograder zip file and attaches a tag. @@ -29,6 +32,8 @@ def build_image(ag_zip_path: str, base_image: str, tag: str): ag_zip_path (``str``): path to the autograder zip file base_image (``str``): base Docker image to build from tag (``str``): tag to be added when creating the image + config (``otter.run.run_autograder.autograder_config.AutograderConfig``): config overrides + for the autograder Returns: ``str``: the tag of the newly-build Docker image @@ -42,13 +47,23 @@ def build_image(ag_zip_path: str, base_image: str, tag: str): with zipfile.ZipFile(ag_zip_path, 'r') as zip_ref: zip_ref.extractall(temp_dir) + # Update the otter_config.json file from the autograder zip with the provided config + # overrides. + config_path = pathlib.Path(temp_dir) / "otter_config.json" + old_config = AutograderConfig() + if config_path.exists(): + old_config = AutograderConfig(json.loads(config_path.read_text("utf-8"))) + + old_config.update(config.get_user_config()) + config_path.write_text(json.dumps(old_config.get_user_config())) + docker.build( temp_dir, build_args={"BASE_IMAGE": base_image}, tags=[image], file=dockerfile_path, load=True, - platforms=[DOCKER_PLATFORM], + # platforms=[DOCKER_PLATFORM], ) return image @@ -60,6 +75,7 @@ def launch_containers( num_containers: int, base_image: str, tag: str, + config: AutograderConfig, **kwargs, ): """ @@ -75,6 +91,8 @@ def launch_containers( num_containers (``int``): number of containers to run in parallel base_image (``str``): the name of a base image to use for building Docker images tag (``str``): a tag to use for the ``otter-grade`` image created for this assignment + config (``otter.run.run_autograder.autograder_config.AutograderConfig``): config overrides + for the autograder **kwargs: additional kwargs passed to ``grade_submission`` Returns: @@ -83,12 +101,16 @@ def launch_containers( """ pool = ThreadPoolExecutor(num_containers) futures = [] - image = build_image(ag_zip_path, base_image, tag) + image = build_image(ag_zip_path, base_image, tag, config) for subm_path in submission_paths: - futures += [ - pool.submit(grade_submission, submission_path=subm_path, image=image, **kwargs) - ] + futures += [pool.submit( + grade_submission, + submission_path=subm_path, + image=image, + # config=config, + **kwargs, + )] # stop execution while containers are running finished_futures = wait(futures) @@ -141,7 +163,8 @@ def grade_submission( if pdf_dir: volumes.append((pdf_path, f"/autograder/submission/{nb_name}.pdf")) - args = {'platform': DOCKER_PLATFORM} + # args = {'platform': DOCKER_PLATFORM} + args = {} if network is not None and not network: args['networks'] = 'none' From e2c25be8c4debbdfafc5cb8e37955d3e252ce570 Mon Sep 17 00:00:00 2001 From: Chris Pyles Date: Sat, 15 Jul 2023 21:49:48 -0700 Subject: [PATCH 2/5] remove Docker platform specification --- otter/grade/containers.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/otter/grade/containers.py b/otter/grade/containers.py index ef702f5bb..a92e39f00 100644 --- a/otter/grade/containers.py +++ b/otter/grade/containers.py @@ -20,7 +20,6 @@ from ..utils import loggers -DOCKER_PLATFORM = "linux/amd64" LOGGER = loggers.get_logger(__name__) @@ -63,7 +62,6 @@ def build_image(ag_zip_path: str, base_image: str, tag: str, config: AutograderC tags=[image], file=dockerfile_path, load=True, - # platforms=[DOCKER_PLATFORM], ) return image @@ -163,7 +161,6 @@ def grade_submission( if pdf_dir: volumes.append((pdf_path, f"/autograder/submission/{nb_name}.pdf")) - # args = {'platform': DOCKER_PLATFORM} args = {} if network is not None and not network: args['networks'] = 'none' From 1834c90d87a92d7238171494c08b20c4c9d7b606 Mon Sep 17 00:00:00 2001 From: Chris Pyles Date: Sat, 15 Jul 2023 21:50:49 -0700 Subject: [PATCH 3/5] changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2ffd2664..65c61ea92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ * Enabled Otter Generate for all assignments created with Otter Assign * Updated Otter Assign to use Otter Run to validate that all tests in the assignment pass, allowing tests to be run for R assignments as well as Python, per [#427](https://github.com/ucbds-infra/otter-grader/issues/427) * Allow Markdown cells to be used instead of raw cells for Otter Assign per [#592](https://github.com/ucbds-infra/otter-grader/issues/592) +* Use Otter Grade CLI flags to update `otter_config.json` values in the grading container per [#395](https://github.com/ucbds-infra/otter-grader/issues/395) +* Removed `linux/amd64` platform specification for Docker images in Otter Grade **v4.4.0:** From a14b5cdcf53791633940d92db2bc96bbfe1b610b Mon Sep 17 00:00:00 2001 From: Chris Pyles Date: Sun, 16 Jul 2023 10:07:34 -0700 Subject: [PATCH 4/5] added tests for CLI overrides --- test/conftest.py | 3 +- test/test_grade/test_integration.py | 80 +++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/test/conftest.py b/test/conftest.py index f2d1076ab..8867a57c0 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -7,7 +7,7 @@ from unittest import mock from otter import __file__ as OTTER_PATH -from otter.grade.containers import build_image, DOCKER_PLATFORM +from otter.grade.containers import build_image from .utils import TestFileManager @@ -88,7 +88,6 @@ def build_image_with_local_changes(*args, **kwargs): tags=[image], file=FILE_MANAGER.get_path("Dockerfile"), load=True, - platforms=[DOCKER_PLATFORM], ) return image diff --git a/test/test_grade/test_integration.py b/test/test_grade/test_integration.py index 6bed7a134..9d4269e3b 100644 --- a/test/test_grade/test_integration.py +++ b/test/test_grade/test_integration.py @@ -6,6 +6,7 @@ import pytest import re import shutil +import zipfile from glob import glob from unittest import mock @@ -20,6 +21,7 @@ ASSIGNMENT_NAME = "otter-grade-test" FILE_MANAGER = TestFileManager(__file__) AG_ZIP_PATH = FILE_MANAGER.get_path("autograder.zip") +ZIP_SUBM_PATH = "test/subm.zip" @pytest.fixture(autouse=True) @@ -30,6 +32,8 @@ def cleanup_output(cleanup_enabled): os.remove("test/final_grades.csv") if os.path.exists("test/submission_pdfs"): shutil.rmtree("test/submission_pdfs") + if os.path.exists(ZIP_SUBM_PATH): + os.remove(ZIP_SUBM_PATH) @pytest.fixture(autouse=True, scope="module") @@ -205,3 +209,79 @@ def test_single_notebook_grade(mocked_launch_grade): mocked_launch_grade.assert_called_with(notebook_path, [notebook_path], **kw_expected) assert output == 1.0 + + +@mock.patch("otter.grade.launch_containers") +def test_config_overrides(mocked_launch_grade): + """ + Checks that the CLI flags are converted to config overrides correctly. + """ + mocked_launch_grade.return_value = [pd.DataFrame([{ + "q1": 2.0, + "q2": 2.0, + "q3": 2.0, + "q4": 1.0, + "q6": 5.0, + "q2b": 2.0, + "q7": 1.0, + "percent_correct": 1.0, + "file": "passesAll.ipynb", + }])] + + notebook_path = FILE_MANAGER.get_path("notebooks/passesAll.ipynb") + grade( + name = "foo", + paths = [notebook_path], + output_dir = "test/", + # the value of the autograder argument doesn't matter, it just needs to be a valid file path + autograder = notebook_path, + containers = 1, + pdfs = True, + ext = "zip", + debug = True, + ) + + assert mocked_launch_grade.call_args.kwargs["config"].get_user_config() == { + "zips": True, + "pdf": True, + "debug": True, + } + + +def test_config_overrides_integration(): + """ + Checks that overriding otter_config.json configurations with CLI flags works. + """ + notebook_path = FILE_MANAGER.get_path("notebooks/passesAll.ipynb") + with zipfile.ZipFile(ZIP_SUBM_PATH, "x") as zf: + zf.write(notebook_path, arcname="passesAll.ipynb") + + output = grade( + name = "foo", + paths = [ZIP_SUBM_PATH], + output_dir = "test/", + # the value of the autograder argument doesn't matter, it just needs to be a valid file path + autograder = AG_ZIP_PATH, + ext = "zip", + ) + + assert output == 1.0 + + got = pd.read_csv("test/final_grades.csv") + want = pd.DataFrame([{ + "q1": 0.0, + "q2": 2.0, + "q3": 2.0, + "q4": 1.0, + "q6": 5.0, + "q2b": 2.0, + "q7": 1.0, + "percent_correct": 1.0, + "file": ZIP_SUBM_PATH, + }]) + + # Sort the columns by label so the dataframes can be compared with ==. + got = got.reindex(sorted(got.columns), axis=1) + want = want.reindex(sorted(want.columns), axis=1) + + assert got.equals(want) From dc1ba7955662627f7e73c895cda39df4ece38688 Mon Sep 17 00:00:00 2001 From: Chris Pyles Date: Sun, 16 Jul 2023 10:46:15 -0700 Subject: [PATCH 5/5] fix test_single_notebook_grade --- test/test_grade/test_integration.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/test_grade/test_integration.py b/test/test_grade/test_integration.py index 9d4269e3b..10ffab915 100644 --- a/test/test_grade/test_integration.py +++ b/test/test_grade/test_integration.py @@ -13,6 +13,7 @@ from otter.generate import main as generate from otter.grade import main as grade +from otter.run.run_autograder.autograder_config import AutograderConfig from otter.utils import loggers from ..utils import TestFileManager @@ -193,7 +194,8 @@ def test_single_notebook_grade(mocked_launch_grade): "no_kill": False, "pdf_dir": None, "timeout": None, - "network": True + "network": True, + "config": AutograderConfig(), } mocked_launch_grade.return_value = [df]