Skip to content

Extract parsing, units and criteria into lambda-feedback-packages - #280

Draft
m-messer wants to merge 27 commits into
mainfrom
refactor/adopt-packages
Draft

Extract parsing, units and criteria into lambda-feedback-packages#280
m-messer wants to merge 27 commits into
mainfrom
refactor/adopt-packages

Conversation

@m-messer

@m-messer m-messer commented Oct 13, 2026

Copy link
Copy Markdown
Member

Problem

The parsing, units and criteria code in app/utility/ (about 3,300 lines) couldn't be reused by other evaluation functions. This PR extracts that code into lambda-feedback-packages and moves the function onto the packages. Both were developed together, against this function's test suite. Structural refactors will follow in a stacked PR.

Changes

  • Packages: lambdafeedback-slr-parsing, -criteria, -expression-parsing and -units, pinned at v0.3.0-rc.2. app/utility/ and the latex2sympy fork are removed, and the unit tests for the extracted code moved to the packages.
  • App: the contexts are rewritten against the package APIs. Feedback text, FeedbackTags, the "written as" forms, EvaluationResult and the previews stay in the app.
  • Healthcheck: new app/evaluation_tests.py lets the base image's healthcheck run the pytest suite. Before, it ran none of our tests. Tests marked @pytest.mark.slow are left out of it, which takes the run from about 35s to about 12s.
  • Tidy-up:
    • Fixed the Dockerfile, whose per-file COPY list was missing modules.
    • Stopped about 1,050 tests running twice.
    • Stopped tests writing files into the repo.
    • Removed dead code and fixed lint.
    • Added a devcontainer and updated the docs.

Behaviour changes (from package fixes)

  • Fewer redundant parentheses in the LaTeX output.
  • With strict syntax, {x+y}*{x+z} gives PARSE_ERROR feedback. It used to crash.
  • Previews of symbols that have no latex defined now render.
  • A reverted unit inside a group is reported once, not twice.
  • A preview no longer mutates the params dict it's given.

Before merging

  • test_radians_to_frequency fails (known, waiting on confirmation). The healthcheck will report DEGRADED until it's fixed.
  • Merge on v0.3.0-rc.2 or wait for v0.3.0.

Testing

  • pytest: 1,211 passed, plus the 1 known failure above.
  • flake8 is clean.
  • The production image builds, and the base image's smoke tests pass.

🤖 Generated with Claude Code

m-messer and others added 27 commits September 11, 2026 20:34
@m-messer @claude
Ignore extracted compareExpressions-packages nested repo
1082321 
The reusable packages extracted from this function live in
/compareExpressions-packages/ as their own separate git repository. Ignore it
so it is not accidentally added as an embedded repo/gitlink.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XP5RCStr93ME5UX4fzyj7y
@m-messer @claude
Depend on the extracted compareexpressions packages
710a92a 
Add the four packages (slr_parsing, criteria, expression_parsing, units) as
git requirements on the compareExpressions-packages repo at v0.2.0. All four
are listed because they depend on each other by name and are not on PyPI.
Nothing uses them yet; the following commits move the app over.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnD49HmKpYCXbgnY3V4GMJ
@m-messer @claude
Use compareexpressions.criteria and slr_parsing; add an app-local result
c88110e 
Criteria graphs, the criteria parser and the physical-quantity criteria
grammar now come from the compareexpressions-criteria and -slr-parsing
packages. The retired EvaluationResult utility is replaced by app/result.py,
which satisfies the criteria package's ResultLike protocol and serialises to
the same response as before (blank feedback skipped, tags always present).
Criteria feedback goes through criteria.add_feedback_from_tags and the test
data through criteria.criteria_test_data.

The criteria parser no longer appends the reserved expressions to a
module-level token list on every evaluation.

Across the app suite every response is unchanged except the criteria-graph
test data: node summaries print `tags: {}` rather than `tags:  {}`, and
mermaid edges are listed in graph order (both in the packages' changelogs).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnD49HmKpYCXbgnY3V4GMJ
@m-messer @claude
Use compareexpressions.expression_parsing for symbolic expressions
60e58b5 
Preprocessing, SymPy parsing, LaTeX parsing, the symbolic preview and the
syntactical forms now come from compareexpressions-expression-parsing:

- Parsing parameters are a SympyParsingConfig built from the evaluation
  parameters; variants use .replace() instead of deep-copied dicts.
- Preprocessing returns a Preprocessed; its FeedbackTag is turned into
  text by feedback.symbolic.internal_feedback, with the same tags as before.
- The JSON parameter defaults move into context/symbolic.py.
- The physical-quantity context uses units.preprocess_quantity, so every
  context's preprocessing step has the same shape.
- The tests pass plain dicts instead of the removed Params TypedDict.

The package no longer mutates the caller's parameters. Before, a preview
set `rationalise: False` and added a `lamda` symbol to the dict it was
given, which leaked into later calls sharing that dict; the responses
don't change as a result.

Behaviour changes, from the package's bug fixes:
- With strict syntax, arithmetic on `{}` set literals is rejected.
  Evaluating `{x+y}*{x+z}` used to crash with AttributeError; it now gives
  PARSE_ERROR feedback, and its preview raises instead of rendering set
  braces. test_curly_braces_not_converted_with_strict_syntax now expects
  the error.
- The preview works for symbols defined without `latex` (it raised
  KeyError, so response_latex fell back to the raw response).
- Preview failures raise ExpressionParsingError, a ValueError.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnD49HmKpYCXbgnY3V4GMJ
@m-messer @claude
Use compareexpressions.units for physical quantities
93909cd 
Quantity parsing, the unit data and the quantity preview now come from
compareexpressions-units, and app/utility and app/preview_implementations
are deleted: nothing uses the copies any more.

- The physical-quantity context's parsing parameters hold a
  SympyParsingConfig, the QuantityParams used to read the response and
  answer, and the tolerances, which used to ride in the parsing dict.
  QuantityParams is built from the same subset of parameters the old
  parser saw, so `symbols`, `units_string` and `is_latex` still don't
  apply when reading quantities during evaluation (unchanged behaviour).
- REVERTED_UNIT FeedbackTags are turned into text by
  feedback.physical_quantity.internal_feedback.
- The unit tests of the removed modules were ported to the packages
  (expression_utilities, multi_character, syntactical_comparison,
  symbolic_preview, slr_quantity, physical_quantity_preview) and are
  deleted here. slr_quantity_test's case lists, which the evaluation tests
  share, move to tests/physical_quantity_test_cases.py.
- typing_extensions is no longer needed.

Behaviour change, from the slr_parsing fix to GROUP traversal: a reverted
unit inside a group is reported once instead of twice, so responses such
as `(kilogrammegametre^2)/(fs^4daA)` get one fewer duplicate
REVERTED_UNIT message.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnD49HmKpYCXbgnY3V4GMJ
@m-messer @claude
Drop the lambda-feedback latex2sympy fork
377cfbb 
latex2sympy2 now comes from PyPI through compareexpressions-expression-
parsing, which the packages were built and tested against. The fork's
relevant change, reading `x = 2` as an equation rather than an assignment,
is reproduced by the package's parse_latex.

The app suite's responses are identical on both builds. A targeted probe
of LaTeX inputs (derivatives, equations, \pm, relations, norms, units,
sums, integrals) only differs for `\mathrm{T}`, which PyPI's release
reads as `T`:
- `5~\mathrm{T}` previews as `5*T` (the fork gave the invalid
  `5*\mathrm{T}`), and physical-quantity evaluation of such LaTeX no longer
  fails with PARSE_ERROR.
- `x^{\mathrm{T}}` is read as a transpose and previews as `x`; the fork
  gave the invalid `x**\mathrm{T}`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnD49HmKpYCXbgnY3V4GMJ
@m-messer @claude
Document the compareexpressions packages
0c70e71 
Point the developer docs at the packages instead of the removed
app/utility and app/preview_implementations modules, describe what each
package provides and how feedback tags become text, and explain how to
develop against a local checkout of the packages.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnD49HmKpYCXbgnY3V4GMJ
@m-messer @claude
Install the packages from m-messer/compare-expressions-packages
6accd12 
The packages repo is published at github.com/m-messer/compare-expressions-
packages, not under lambda-feedback, and has no v0.2.0 tag. Pin all four
packages to the 0.2.0 commit on its main branch (fe90efe) and point the
docs at the real repository.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnD49HmKpYCXbgnY3V4GMJ
@m-messer
"Added physical quantity preview implementation and associated tests"
6e31934
@m-messer @claude
Add symbolic_preview.py and feedback/tags.py, moved from expression_p…
572c547 
…arsing

expression_parsing 0.3.0 drops preview_function, parse_symbolic, Preview,
Result, ExpressionSyntaxError, FeedbackTag and FeedbackTagName: rendering a
response for display, and mapping a parse failure to a learner-facing tag,
are app-layer concerns. Not yet wired up to anything (next commit).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@m-messer @claude
Wire up the moved symbolic preview in preview.py and context/symbolic.py
f0551d1 
Both imported preview_function/Result from compareexpressions.expression_parsing;
point them at the new local symbolic_preview module instead.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@m-messer @claude
Rebuild physical_quantity_preview.py against the new expression_parsi…
416aed0 
…ng/units APIs

Preview/Result now come from the local symbolic_preview module (expression_parsing
no longer has them), and preprocess_quantity returns a plain string instead of
a Preprocessed with .expression.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@m-messer @claude
Catch typed notation exceptions in evaluation.py instead of checking …
a8f1837 
…Preprocessed.success

expression_parsing's preprocess_expression (and units' preprocess_quantity,
which never raises these) now raises BracketNotationError/AbsoluteValueNotationError
instead of returning a Preprocessed(expression, feedback). Both call sites
become try/except, reproducing the same warn-for-learner/fatal-for-answer
split as before.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@m-messer
Update physical_quantity to handle reverted_unit feedback
c48fe3a 
Replaced `internal_feedback` with `reverted_unit_feedback` and updated the feedback mechanism to support `reverted_units`, adding detailed contextual information.
@m-messer
Replace add_feedback_from_tags with CriteriaGraph.export_feedback…
53ac036 
… for improved feedback handling

Updated `evaluation.py` and `result.py` to switch from `add_feedback_from_tags` to `CriteriaGraph.export_feedback`. Streamlined feedback generation and modified related test data serialization logic.
@m-messer @claude
Pin the packages at 2848e58 (CriteriaGraph.export_feedback)
686d3d9 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@m-messer @claude
Use the renamed lambdafeedback packages
541162c 
The extracted packages are now `lambdafeedback-*` (imports
`lambdafeedback.<pkg>`) in m-messer/lambda-feedback-packages. Pin them at
865d399 and update the imports and docs. The packages checkout no longer
lives inside this repo, so drop its .gitignore entry.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@m-messer @claude
Ignore extracted compareExpressions-packages nested repo
3f4dff3 
The reusable packages extracted from this function live in
/compareExpressions-packages/ as their own separate git repository. Ignore it
so it is not accidentally added as an embedded repo/gitlink.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XP5RCStr93ME5UX4fzyj7y
@m-messer
Merge branch 'main' into refactor/adopt-packages
08454d7
@m-messer
Refactor symbolic and quantity contexts for updated lambdafeedback APIs
8e27807 
- Replace `attach` calls with `attach_new_criterion` or `attach_new_evaluation`.
- Introduce helper for unsimplified parsing with unsplittable symbols.
- Add `syntactical_forms.py` for number form patterns.
- Update `requirements.txt` to lambdafeedback v0.3.0-rc.1.
@m-messer
Simplify LaTeX outputs: Remove unnecessary parentheses and update not…
bd9c991 
…ations across tests for consistency.
@m-messer @claude
Use SympyParsingConfig.replace for extra unsplittable symbols
9807764 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@m-messer
Update lambdafeedback dependencies to v0.3.0-rc.2
8f0c5d0
@m-messer
Remove unused benchmarking scripts, Mermaid documentation files, and …
c7d375b 
…redundant test code
@m-messer
Add development container setup with Dockerfile and configuration files
f7a1b32 
- Introduce `.devcontainer/devcontainer.json` and `.devcontainer/Dockerfile` for containerized development.
- Include a `.dockerignore` file to specify ignored files during build.
- Configure Python, Node.js, and `claude` CLI in the devcontainer for local sandboxed testing.
@m-messer
Remove redundant pytest.main calls, outdated imports, and update do…
7a091d9 
…cumentation for tests and Docker workflow
@m-messer
Mark slow-running tests with @pytest.mark.slow and update healthche…
8e95025 
…ck logic to exclude them
@m-messer m-messer mentioned this pull request Oct 14, 2026
Draft
1 of 2 tasks

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@m-messer