Fix the entry points left unadjusted from the template - #21
Open
peterbjohnson wants to merge 2 commits into
Open
Fix the entry points left unadjusted from the template#21peterbjohnson wants to merge 2 commits into
peterbjohnson wants to merge 2 commits into
Conversation
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 "<br>", 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 <noreply@anthropic.com>
peterbjohnson
force-pushed
the
fix_boilerplate_entrypoints
branch
from
September 9, 2026 20:55
318d899 to
86a864c
Compare
ada-jz7125
approved these changes
Sep 10, 2026
ada-jz7125
left a comment
Collaborator
There was a problem hiding this comment.
There are text assertions in preview_test.py, but I think this keyword assertion is acceptable as long as the preview is not changed completely
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #14
Four defects inherited from the boilerplate. None affected the deployed eval path, but each misled anyone working on the repo.
Approach
Tests first, in
dev_test.py(new, following the existing<module>_test.pyconvention) and a rewrittenpreview_test.py. Before the change:The one pass was the old preview test, which asserted only that a preview key existed and was not None. That is why the sympy echo survived this long.
1.
dev.pyfailed immediatelyThe command the README pointed at:
Two faults. It called
.to_dict()on a value that is a plain dict, and it passed its arguments as(answer, response)while the signature is(response, answer). The argument swap is the nastier one: it does not raise, it just silently exchanges "missing" and "extra" in the feedback. It now prints the result as JSON and passes the arguments the right way round.2.
preview.pywas the sympy placeholderIt echoed the response back under a
sympykey, which is meaningless for MIDI. It now reports what the system actually read: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
Previewtype carries onlysympyandfeedback, so the summary goes infeedback.I implemented this rather than removing the preview registration, which #12 offered as the alternative. Removing it would mean
command: previewstarts returning an error, and I would not want to change what the platform sees on the strength of a maintenance pass. Say the word if you would rather it went.3. Wrong return annotation in
evaluation.pyAnnotated
-> Result, referring to lf_toolkit'sResultclass, which this function has never returned. Worth noting this is not merely cosmetic: actually returning aResultwould change the output, because that class renders feedback by joining items with<br>, which would mangle the newline-separated message this function produces. The dict is correct, so the annotation is what changes. Three tests in section 8 now pin the returned shape.4. Healthcheck failed inside the container
command: healthcheckruns the test suite, andevaluation_test.pyreads its bulk cases fromdata/, which the image did not carry:.dockerignoreexcluded the wholedata/directory, so simply adding aCOPYfailed the build outright. It now admits that one fixture and nothing else, so datasets and recordings that land indata/still stay out of the image. The fixture load also tolerates the file being absent, rather than failing the whole module at import and taking every other test down with it.Verified in a container built from this branch, with #12's backend fix layered on so it would boot on arm64:
tests_passedfalsetrueevaluation_function.evaluation_testOnly the 56 KB fixture lands in
/app/data.Verification
Note on test file placement
dev_test.pyis a new file, but it mirrorsdev.pyexactly aspreview_test.pymirrorspreview.py, so it follows the convention already in the repo rather than adding another ad-hoc one. The return-shape tests went into section 8 ofevaluation_test.py, where the otherevaluation_functionintegration tests live, deliberately avoiding a new numbered section that would collide with #13's section 11.🤖 Generated with Claude Code