From 86a864cdfe6482c11bade0ff01aa9751e6ea86df Mon Sep 17 00:00:00 2001 From: "Peter B. Johnson" Date: Wed, 9 Sep 2026 21:52:14 +0100 Subject: [PATCH 1/2] Fix the entry points left unadjusted from the template Four defects inherited from the boilerplate. None affected the deployed eval path, but each misled anyone working on the repo. dev.py: the documented command failed immediately. It called .to_dict() on a value that is a plain dict, and passed its arguments as (answer, response) while the function's signature is (response, answer), which silently swaps "missing" and "extra" in the feedback. It now prints the result as JSON and passes the arguments the right way round. preview.py: returned the response echoed back under a "sympy" key, which is meaningless for MIDI. It now reports what was read: note count, duration and pitch range. An audio path is named but deliberately not transcribed, because the preview runs while the student is still working and transcription takes seconds. The platform's Preview type carries only "sympy" and "feedback", so the summary goes in "feedback". evaluation.py: annotated as returning lf_toolkit's Result class, which it has never done. Returning one would change the output, because that class renders feedback by joining items with "
", which would mangle the newline-separated message. The dict is correct, so the annotation is what changes. healthcheck: the command runs the test suite, and evaluation_test.py reads its bulk cases from data/, which the image did not carry. Copy the fixture in, and tolerate its absence rather than failing the whole module at import. .dockerignore excluded the directory outright, so it now admits that one file and nothing else. Co-Authored-By: Claude Opus 5 --- .dockerignore | 7 ++- Dockerfile | 4 ++ evaluation_function/dev.py | 41 ++++++++---- evaluation_function/dev_test.py | 81 ++++++++++++++++++++++++ evaluation_function/evaluation.py | 6 +- evaluation_function/evaluation_test.py | 33 ++++++++-- evaluation_function/preview.py | 86 ++++++++++++++++++++------ evaluation_function/preview_test.py | 84 +++++++++++++++++++------ 8 files changed, 284 insertions(+), 58 deletions(-) create mode 100644 evaluation_function/dev_test.py diff --git a/.dockerignore b/.dockerignore index 4dc8d42..0175694 100644 --- a/.dockerignore +++ b/.dockerignore @@ -140,8 +140,11 @@ README.md # GitHub .github -# Data folder -data/ +# Data folder: keep it out of the image apart from the test fixture, which +# the healthcheck needs because that command runs the test suite. Datasets +# and recordings that land here must not be baked into the image. +data/* +!data/longMIDIsequence.json # Test reports reports/ \ No newline at end of file diff --git a/Dockerfile b/Dockerfile index ffe9617..b1cb01f 100755 --- a/Dockerfile +++ b/Dockerfile @@ -25,6 +25,10 @@ RUN python -m compileall -q . # Copy the evaluation function to the app directory COPY evaluation_function ./evaluation_function +# The test fixtures are needed too: the healthcheck command runs the test +# suite, and evaluation_test.py reads its bulk cases from here. +COPY data ./data + # Command to start the evaluation function with ENV FUNCTION_COMMAND="python" diff --git a/evaluation_function/dev.py b/evaluation_function/dev.py index 886d641..eff3eb8 100644 --- a/evaluation_function/dev.py +++ b/evaluation_function/dev.py @@ -1,24 +1,43 @@ +""" +dev.py +====== +Command line entry point, for trying the evaluation function without +running the server. + +Usage: + python -m evaluation_function.dev '' '' + +Both arguments are JSON, in the same shape the platform sends, for example: + '{"notes": [{"pitch": 60, "start": 0.0, "duration": 0.5}]}' +""" + +import json import sys from lf_toolkit.shared.params import Params from .evaluation import evaluation_function -def dev(): - """Run the evaluation function from the command line for development purposes. +USAGE = "Usage: python -m evaluation_function.dev '' ''" - Usage: python -m evaluation_function.dev - """ + +def dev(): + """Run the evaluation function once and print the result.""" if len(sys.argv) < 3: - print("Usage: python -m evaluation_function.dev ") + print(USAGE) return - - answer = sys.argv[1] - response = sys.argv[2] - result = evaluation_function(answer, response, Params()) + # Argument order matches evaluation_function itself: the student's + # response first, the reference answer second. + response = sys.argv[1] + answer = sys.argv[2] + + result = evaluation_function(response, answer, Params()) + + # evaluation_function returns a plain dict, so print it as JSON rather + # than calling a serialisation method it does not have. + print(json.dumps(result, indent=2)) - print(result.to_dict()) if __name__ == "__main__": - dev() \ No newline at end of file + dev() diff --git a/evaluation_function/dev_test.py b/evaluation_function/dev_test.py new file mode 100644 index 0000000..8e96ccd --- /dev/null +++ b/evaluation_function/dev_test.py @@ -0,0 +1,81 @@ +""" +dev_test.py +=========== +Tests for the command line entry point in dev.py. + +This is the command the README points developers at, so it should at least +run. It was inherited from the template and never adjusted: it called a +method that does not exist on the returned value, and passed its two +arguments the wrong way round. + +Run locally with: python -m pytest evaluation_function/dev_test.py -v +""" + +import json +import sys + +from . import dev as dev_module +from .dev import dev +from .evaluation_test import make_midi + + +# Helpers +# ------------------------------------------------------------------------------ +TWO_NOTES = json.dumps(make_midi([60, 62], [0.0, 0.5], [0.4, 0.4])) +THREE_NOTES = json.dumps(make_midi([60, 62, 64], [0.0, 0.5, 1.0], [0.4, 0.4, 0.4])) + + +# Tests +# ------------------------------------------------------------------------------ +def test_prints_a_result(monkeypatch, capsys): + """The documented invocation must run and print the outcome.""" + monkeypatch.setattr(sys, "argv", ["dev", TWO_NOTES, TWO_NOTES]) + + dev() + + printed = capsys.readouterr().out + assert "is_correct" in printed + assert "feedback" in printed + + +def test_reports_a_matching_performance_as_correct(monkeypatch, capsys): + monkeypatch.setattr(sys, "argv", ["dev", TWO_NOTES, TWO_NOTES]) + + dev() + + # Printed as JSON, so the boolean is lowercase. + assert '"is_correct": true' in capsys.readouterr().out + + +def test_passes_arguments_in_response_then_answer_order(monkeypatch): + """ + The first argument is the student's response and the second is the + reference answer, matching evaluation_function's own signature. Getting + this backwards silently swaps "missing" and "extra" in the feedback. + """ + seen = {} + + def spy(response, answer, params): + seen["response"] = response + seen["answer"] = answer + return {"is_correct": True, "feedback": ""} + + monkeypatch.setattr(dev_module, "evaluation_function", spy) + monkeypatch.setattr(sys, "argv", ["dev", TWO_NOTES, THREE_NOTES]) + + dev() + + assert seen["response"] == TWO_NOTES + assert seen["answer"] == THREE_NOTES + + +def test_usage_message_when_arguments_are_missing(monkeypatch, capsys): + monkeypatch.setattr(sys, "argv", ["dev"]) + + dev() + + printed = capsys.readouterr().out + assert "usage" in printed.lower() + # Response first, then answer. The arguments are quoted because the + # JSON they carry contains spaces and braces. + assert printed.index("") < printed.index("") diff --git a/evaluation_function/evaluation.py b/evaluation_function/evaluation.py index a8485d9..fbc4e9c 100755 --- a/evaluation_function/evaluation.py +++ b/evaluation_function/evaluation.py @@ -7,8 +7,8 @@ """ import json -from typing import Any -from lf_toolkit.evaluation import Result, Params +from typing import Any, Dict +from lf_toolkit.shared.params import Params from .compare_MIDI import ( compare_performance_ED, @@ -61,7 +61,7 @@ def evaluation_function( response: Any, answer: Any, params: Params, -) -> Result: +) -> Dict[str, Any]: """ Function used to evaluate a student response. --- diff --git a/evaluation_function/evaluation_test.py b/evaluation_function/evaluation_test.py index 9aca78e..7553e3d 100755 --- a/evaluation_function/evaluation_test.py +++ b/evaluation_function/evaluation_test.py @@ -587,6 +587,26 @@ def test_pitch_error_is_not_correct(self): result = evaluation_function(res, ref, {}) assert result["is_correct"] == False + # Shimmy serialises whatever this function returns straight to JSON, so + # the return value must be a plain dict carrying these two keys. The + # template annotated it as returning lf_toolkit's Result class, which it + # has never done, and which renders feedback by joining items with + # "
" -- that would mangle the newline-separated message produced here. + def test_returns_a_plain_dict(self): + midi = make_midi([60, 62], [0.0, 0.5], [0.4, 0.4]) + assert type(evaluation_function(midi, midi, {})) is dict + + def test_carries_is_correct_and_feedback(self): + midi = make_midi([60, 62], [0.0, 0.5], [0.4, 0.4]) + result = evaluation_function(midi, midi, {}) + assert isinstance(result["is_correct"], bool) + assert isinstance(result["feedback"], str) + + def test_result_is_json_encodable(self): + import json as _json + midi = make_midi([60, 62], [0.0, 0.5], [0.4, 0.4]) + _json.dumps(evaluation_function(midi, midi, {})) + # 9. Tests for parameter overrides # ------------------------------------------------------------------------------ @@ -647,10 +667,15 @@ def test_custom_chord_onset_window_affects_grouping(self): root_dir = os.path.dirname(this_dir) # compareMusic/ path = os.path.join(root_dir, "data", "longMIDIsequence.json") -with open(path, "r") as json_file: - REALISTIC_TEST_DATA = json.load(json_file) - -REALISTIC_TEST_CASES = REALISTIC_TEST_DATA["test_cases"] +# The fixture lives outside the package, so it is not guaranteed to be present +# everywhere the tests run. Degrade to skipping these cases rather than failing +# the whole module at import, which would take every other test down with it. +if os.path.exists(path): + with open(path, "r") as json_file: + REALISTIC_TEST_DATA = json.load(json_file) + REALISTIC_TEST_CASES = REALISTIC_TEST_DATA["test_cases"] +else: + REALISTIC_TEST_CASES = [] REALISTIC_TEST_IDS = [case["name"] for case in REALISTIC_TEST_CASES] @pytest.mark.parametrize("case", REALISTIC_TEST_CASES, ids=REALISTIC_TEST_IDS) diff --git a/evaluation_function/preview.py b/evaluation_function/preview.py index 007f997..328a090 100755 --- a/evaluation_function/preview.py +++ b/evaluation_function/preview.py @@ -1,28 +1,78 @@ +""" +preview.py +========== +Preview shown to the student before they submit. + +Its job is to confirm what the system read from their submission, so that +a wrong file or an empty recording is caught before it is marked. It runs +while the student is still working, so it must stay fast: in particular it +never transcribes audio, which takes seconds. + +The platform's Preview type carries only "sympy" and "feedback", so the +summary goes in "feedback" as a short line of text. +""" + +import json +import os from typing import Any + from lf_toolkit.preview import Result, Params, Preview -def preview_function(response: Any, params: Params) -> Result: - """ - Function used to preview a student response. - --- - The handler function passes three arguments to preview_function(): +from .audio_processing import AUDIO_EXTENSIONS +from .compare_MIDI import PITCH_CLASS_NAMES + + +def note_name(pitch): + """Convert a MIDI pitch number to a name, e.g. 60 -> "C4".""" + return PITCH_CLASS_NAMES[pitch % 12] + str(pitch // 12 - 1) + - - `response` which are the answers provided by the student. - - `params` which are any extra parameters that may be useful, - e.g., error tolerances. +def summarise_notes(notes): + """One line describing a list of notes: how many, how long, what range.""" + if not notes: + return "No notes found in this submission." - The output of this function is what is returned as the API response - and therefore must be JSON-encodable. It must also conform to the - response schema. + count = len(notes) + noun = "note" if count == 1 else "notes" - Any standard python library may be used, as well as any package - available on pip (provided it is added to requirements.txt). + end = max(note["start"] + note["duration"] for note in notes) + pitches = [note["pitch"] for note in notes] - The way you wish to structure you code (all in this function, or - split into many) is entirely up to you. + return ( + f"{count} {noun}, {end:.1f} s, " + f"{note_name(min(pitches))} to {note_name(max(pitches))}." + ) + + +def preview_function(response: Any, params: Params) -> Result: """ + Summarise the student's submission without evaluating it. + + Args: + response: the student's submission, as MIDI note data, a JSON string + of the same, or the path to an audio recording. + params: unused here, accepted for interface compatibility. + Returns: + Result carrying a one-line description of what was read. + """ try: - return Result(preview=Preview(sympy=response)) - except Exception as e: - return Result(preview=Preview(feedback=str(e))) + # An audio recording is reported as such. Transcribing it here would + # take seconds, which is far too slow while the student is working. + if isinstance(response, str): + extension = os.path.splitext(response)[1].lower() + if extension in AUDIO_EXTENSIONS: + name = os.path.basename(response) + return Result(preview=Preview( + feedback=f"Audio recording {name}, transcribed on submission." + )) + + response = json.loads(response) + + return Result(preview=Preview(feedback=summarise_notes(response["notes"]))) + + except Exception: + return Result(preview=Preview( + feedback="This submission could not be read as MIDI note data " + "or as an audio recording." + )) diff --git a/evaluation_function/preview_test.py b/evaluation_function/preview_test.py index a8834a7..d008b65 100755 --- a/evaluation_function/preview_test.py +++ b/evaluation_function/preview_test.py @@ -1,29 +1,73 @@ +""" +preview_test.py +=============== +Tests for the preview function. + +The preview is what a student sees before submitting, so it should tell +them what the system read from their submission. The template version +echoed the response back under a "sympy" key, which is meaningless for +MIDI. + +The preview must stay fast, because it runs while the student is still +working. In particular it must not transcribe audio, which takes seconds. + +Run locally with: python -m pytest evaluation_function/preview_test.py -v +""" + +import json import unittest +from .evaluation_test import make_midi from .preview import Params, preview_function -class TestPreviewFunction(unittest.TestCase): - """ - TestCase Class used to test the algorithm. - --- - Tests are used here to check that the algorithm written - is working as it should. - It's best practice to write these tests first to get a - kind of 'specification' for how your algorithm should - work, and you should run these tests before committing - your code to AWS. +def feedback_for(response): + return preview_function(response, Params())["preview"]["feedback"] + + +class TestMidiSubmissions(unittest.TestCase): + + def test_reports_the_note_count(self): + midi = make_midi([60, 62, 64], [0.0, 0.5, 1.0], [0.4, 0.4, 0.4]) + assert "3 notes" in feedback_for(midi) + + def test_reports_the_duration(self): + midi = make_midi([60, 62], [0.0, 2.0], [0.5, 0.5]) + assert "2.5" in feedback_for(midi) + + def test_reports_the_pitch_range(self): + midi = make_midi([60, 72], [0.0, 0.5], [0.4, 0.4]) + feedback = feedback_for(midi) + assert "C4" in feedback + assert "C5" in feedback + + def test_accepts_a_json_string(self): + # The platform sends response and answer as JSON strings. + midi = make_midi([60, 62, 64], [0.0, 0.5, 1.0], [0.4, 0.4, 0.4]) + assert "3 notes" in feedback_for(json.dumps(midi)) + + def test_single_note_is_not_pluralised(self): + midi = make_midi([60], [0.0], [0.4]) + assert "1 note" in feedback_for(midi) + assert "1 notes" not in feedback_for(midi) + + +class TestSubmissionsWithoutNotes(unittest.TestCase): - Read the docs on how to use unittest here: - https://docs.python.org/3/library/unittest.html + def test_empty_note_list_is_reported(self): + assert "no notes" in feedback_for({"notes": []}).lower() - Use preview_function() to check your algorithm works - as it should. - """ + def test_audio_path_is_described_without_transcribing(self): + # Transcription takes seconds, which is far too slow for a preview. + feedback = feedback_for("/uploads/practice.wav") + assert "audio" in feedback.lower() + assert "practice.wav" in feedback - def test_preview(self): - response, params = "A", Params() - result = preview_function(response, params) + def test_unreadable_submission_does_not_raise(self): + feedback = feedback_for("this is not MIDI at all") + assert feedback - self.assertIn("preview", result) - self.assertIsNotNone(result["preview"]) + def test_result_always_has_a_preview(self): + for response in ({"notes": []}, "nonsense", 42, None): + result = preview_function(response, Params()) + assert result["preview"] is not None From 6121639683d52afaf48a2e3059675127cc8f92a2 Mon Sep 17 00:00:00 2001 From: "Peter B. Johnson" Date: Thu, 10 Sep 2026 08:29:55 +0100 Subject: [PATCH 2/2] Pin the preview's fixed messages by constant, not by substring Follows the same review point raised on #19. The two fixed preview messages are now named constants, so tests assert which case was hit rather than how it happens to be worded. The pitch-range test no longer hard-codes note names either. Note naming is a fact about MIDI rather than a wording choice, so it gets its own tests, and the range test now checks that the lowest and highest pitches are the ones reported, and that a middle pitch is not. The remaining text assertions cover note count, duration and pluralisation, which are the preview's actual contract rather than incidental phrasing. Co-Authored-By: Claude Opus 5 --- evaluation_function/preview.py | 15 ++++++++---- evaluation_function/preview_test.py | 37 ++++++++++++++++++++++------- 2 files changed, 39 insertions(+), 13 deletions(-) diff --git a/evaluation_function/preview.py b/evaluation_function/preview.py index 328a090..35c2bcb 100755 --- a/evaluation_function/preview.py +++ b/evaluation_function/preview.py @@ -21,6 +21,14 @@ from .audio_processing import AUDIO_EXTENSIONS from .compare_MIDI import PITCH_CLASS_NAMES +# Fixed messages, named so that tests can assert which case was hit without +# depending on the wording, which is free to change. +NO_NOTES_MESSAGE = "No notes found in this submission." +UNREADABLE_MESSAGE = ( + "This submission could not be read as MIDI note data " + "or as an audio recording." +) + def note_name(pitch): """Convert a MIDI pitch number to a name, e.g. 60 -> "C4".""" @@ -30,7 +38,7 @@ def note_name(pitch): def summarise_notes(notes): """One line describing a list of notes: how many, how long, what range.""" if not notes: - return "No notes found in this submission." + return NO_NOTES_MESSAGE count = len(notes) noun = "note" if count == 1 else "notes" @@ -72,7 +80,4 @@ def preview_function(response: Any, params: Params) -> Result: return Result(preview=Preview(feedback=summarise_notes(response["notes"]))) except Exception: - return Result(preview=Preview( - feedback="This submission could not be read as MIDI note data " - "or as an audio recording." - )) + return Result(preview=Preview(feedback=UNREADABLE_MESSAGE)) diff --git a/evaluation_function/preview_test.py b/evaluation_function/preview_test.py index d008b65..e41307a 100755 --- a/evaluation_function/preview_test.py +++ b/evaluation_function/preview_test.py @@ -18,7 +18,13 @@ import unittest from .evaluation_test import make_midi -from .preview import Params, preview_function +from .preview import ( + NO_NOTES_MESSAGE, + UNREADABLE_MESSAGE, + Params, + note_name, + preview_function, +) def feedback_for(response): @@ -36,10 +42,13 @@ def test_reports_the_duration(self): assert "2.5" in feedback_for(midi) def test_reports_the_pitch_range(self): - midi = make_midi([60, 72], [0.0, 0.5], [0.4, 0.4]) + # The names themselves are pinned by TestNoteName below, so this only + # checks that the lowest and highest pitches are the ones reported. + midi = make_midi([60, 64, 72], [0.0, 0.5, 1.0], [0.4, 0.4, 0.4]) feedback = feedback_for(midi) - assert "C4" in feedback - assert "C5" in feedback + assert note_name(60) in feedback + assert note_name(72) in feedback + assert note_name(64) not in feedback def test_accepts_a_json_string(self): # The platform sends response and answer as JSON strings. @@ -55,7 +64,7 @@ def test_single_note_is_not_pluralised(self): class TestSubmissionsWithoutNotes(unittest.TestCase): def test_empty_note_list_is_reported(self): - assert "no notes" in feedback_for({"notes": []}).lower() + assert feedback_for({"notes": []}) == NO_NOTES_MESSAGE def test_audio_path_is_described_without_transcribing(self): # Transcription takes seconds, which is far too slow for a preview. @@ -63,11 +72,23 @@ def test_audio_path_is_described_without_transcribing(self): assert "audio" in feedback.lower() assert "practice.wav" in feedback - def test_unreadable_submission_does_not_raise(self): - feedback = feedback_for("this is not MIDI at all") - assert feedback + def test_unreadable_submission_is_reported(self): + assert feedback_for("this is not MIDI at all") == UNREADABLE_MESSAGE def test_result_always_has_a_preview(self): for response in ({"notes": []}, "nonsense", 42, None): result = preview_function(response, Params()) assert result["preview"] is not None + + +class TestNoteName(unittest.TestCase): + """Pitch numbering is a fact about MIDI, not a wording choice.""" + + def test_middle_c(self): + assert note_name(60) == "C4" + + def test_octave_above_middle_c(self): + assert note_name(72) == "C5" + + def test_accidental(self): + assert note_name(61) == "C#4"