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..35c2bcb 100755 --- a/evaluation_function/preview.py +++ b/evaluation_function/preview.py @@ -1,28 +1,83 @@ +""" +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 + +# 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".""" + 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. - 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. +def summarise_notes(notes): + """One line describing a list of notes: how many, how long, what range.""" + if not notes: + return NO_NOTES_MESSAGE - Any standard python library may be used, as well as any package - available on pip (provided it is added to requirements.txt). + count = len(notes) + noun = "note" if count == 1 else "notes" - The way you wish to structure you code (all in this function, or - split into many) is entirely up to you. + end = max(note["start"] + note["duration"] for note in notes) + pitches = [note["pitch"] for note in notes] + + 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=UNREADABLE_MESSAGE)) diff --git a/evaluation_function/preview_test.py b/evaluation_function/preview_test.py index a8834a7..e41307a 100755 --- a/evaluation_function/preview_test.py +++ b/evaluation_function/preview_test.py @@ -1,29 +1,94 @@ +""" +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 .preview import Params, preview_function +from .evaluation_test import make_midi +from .preview import ( + NO_NOTES_MESSAGE, + UNREADABLE_MESSAGE, + Params, + note_name, + preview_function, +) + + +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): + # 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 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. + 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): + + def test_empty_note_list_is_reported(self): + 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. + feedback = feedback_for("/uploads/practice.wav") + assert "audio" in feedback.lower() + assert "practice.wav" in feedback + + def test_unreadable_submission_is_reported(self): + assert feedback_for("this is not MIDI at all") == UNREADABLE_MESSAGE -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. + 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 - 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. - Read the docs on how to use unittest here: - https://docs.python.org/3/library/unittest.html +class TestNoteName(unittest.TestCase): + """Pitch numbering is a fact about MIDI, not a wording choice.""" - Use preview_function() to check your algorithm works - as it should. - """ + def test_middle_c(self): + assert note_name(60) == "C4" - def test_preview(self): - response, params = "A", Params() - result = preview_function(response, params) + def test_octave_above_middle_c(self): + assert note_name(72) == "C5" - self.assertIn("preview", result) - self.assertIsNotNone(result["preview"]) + def test_accidental(self): + assert note_name(61) == "C#4"