Add symmath to openedx-calc

[Carlos-Muniz]

closes #37

symmath is leaving edx-platform and moving to openedx-calc. As our first step we move symmath to openedx-calc. We update the requirements, test config files, the readme, the setup.py, and move all the tests to its own directory.

[dianakhuang]

I think this looks good to me overall. I didn't dig into the specific symmath lines of code since I'm assuming that's mostly, if not entirely, copy/paste. I don't think this will impact much until we delete this out of edx-platform.

[kdmccormick]

Code changes looks solid 👍🏻

Two issues with the tests though:

1. I don't think linting is being run. Both the py38 and quality test contexts run unit tests; neither contexts run quality analysis. I think there's a disconnect in the github action -> tox machinery. Unfortunately it looks like this was the case before you opened your PR :(
2. I think test_symmath_check_different_symbols is silently failing

2022-01-13T20:39:52.0945218Z test_symmath_check_different_symbols (tests.test_symmath_check.SymmathCheckTest) ... Err 'lxml.etree._Element' object has no attribute 'decode' while preprocessing; expr=<math xmlns="http://www.w3.org/1998/Math/MathML">
2022-01-13T20:39:52.0952420Z   <mstyle displaystyle="true">
2022-01-13T20:39:52.0952888Z   <mrow>
2022-01-13T20:39:52.0953227Z     <mi>x</mi>
2022-01-13T20:39:52.0953469Z     <mo>+</mo>
2022-01-13T20:39:52.0953758Z     <mi>y</mi>
2022-01-13T20:39:52.0953991Z   </mrow>
2022-01-13T20:39:52.0954305Z </mstyle>
2022-01-13T20:39:52.0954559Z </math>
2022-01-13T20:39:52.0955459Z Error evaluating expression 'x+y' as a valid equation
2022-01-13T20:39:52.0955869Z Traceback (most recent call last):
2022-01-13T20:39:52.0964356Z   File "/home/runner/work/openedx-calc/openedx-calc/symmath/symmath_check.py", line 270, in symmath_check
2022-01-13T20:39:52.0965117Z     fsym = f.sympy
2022-01-13T20:39:52.0965849Z   File "/home/runner/work/openedx-calc/openedx-calc/symmath/formula.py", line 468, in make_sympy
2022-01-13T20:39:52.0966297Z     self.the_sympy = self.make_sympy(xml[0])
2022-01-13T20:39:52.0966775Z   File "src/lxml/etree.pyx", line 1176, in lxml.etree._Element.__getitem__
2022-01-13T20:39:52.0967216Z IndexError: list index out of range
2022-01-13T20:39:52.0967531Z ok
2022-01-13T20:39:52.0996244Z test_symmath_check_equivalent_symbols (tests.test_symmath_check.SymmathCheckTest) ... ok
2022-01-13T20:39:52.9970904Z test_symmath_check_floats (tests.test_symmath_check.SymmathCheckTest) ... ok
2022-01-13T20:39:53.7788603Z test_symmath_check_integers (tests.test_symmath_check.SymmathCheckTest) ... ok
2022-01-13T20:39:53.7984221Z test_symmath_check_same_symbols (tests.test_symmath_check.SymmathCheckTest) ... ok
2022-01-13T20:39:53.7984503Z 
2022-01-13T20:39:53.7985274Z ----------------------------------------------------------------------
2022-01-13T20:39:53.7985902Z Ran 64 tests in 3.923s
2022-01-13T20:39:53.7986042Z 
2022-01-13T20:39:53.7986327Z OK

[kdmccormick]

@Carlos-Muniz Two other thoughts that just came to mind:

Firstly, you will want to bump the version of this project, potentially the major version since this is a relatively large change.

Secondly: by default, edx-platform will upgrade to the latest version of all project every week unless they are explicitly constrainted. So, edx-platform would inherit these changes within a week after merging this PR if you release a new version (which you should). To be honest, I don't know what happens if two versions of symmath (the edx-platform/common/lib version and this one) are both available to import!

So, in edx-platform, you may want to constrain openedx-calc to its current, symmath-less version before merging this PR. Then, once you're ready, you could delete the edx-platform/common/lib copy of symmath and upgrade to the latest version of openedx-calc all in one edx-platform PR.

[Carlos-Muniz]

Code changes looks solid 👍🏻

Two issues with the tests though:

1. I don't think linting is being run. Both the py38 and quality test contexts run unit tests; neither contexts run quality analysis. I think there's a disconnect in the github action -> tox machinery. Unfortunately it looks like this was the case before you opened your PR :(
2. I think test_symmath_check_different_symbols is silently failing

2022-01-13T20:39:52.0945218Z test_symmath_check_different_symbols (tests.test_symmath_check.SymmathCheckTest) ... Err 'lxml.etree._Element' object has no attribute 'decode' while preprocessing; expr=<math xmlns="http://www.w3.org/1998/Math/MathML">
2022-01-13T20:39:52.0952420Z   <mstyle displaystyle="true">
2022-01-13T20:39:52.0952888Z   <mrow>
2022-01-13T20:39:52.0953227Z     <mi>x</mi>
2022-01-13T20:39:52.0953469Z     <mo>+</mo>
2022-01-13T20:39:52.0953758Z     <mi>y</mi>
2022-01-13T20:39:52.0953991Z   </mrow>
2022-01-13T20:39:52.0954305Z </mstyle>
2022-01-13T20:39:52.0954559Z </math>
2022-01-13T20:39:52.0955459Z Error evaluating expression 'x+y' as a valid equation
2022-01-13T20:39:52.0955869Z Traceback (most recent call last):
2022-01-13T20:39:52.0964356Z   File "/home/runner/work/openedx-calc/openedx-calc/symmath/symmath_check.py", line 270, in symmath_check
2022-01-13T20:39:52.0965117Z     fsym = f.sympy
2022-01-13T20:39:52.0965849Z   File "/home/runner/work/openedx-calc/openedx-calc/symmath/formula.py", line 468, in make_sympy
2022-01-13T20:39:52.0966297Z     self.the_sympy = self.make_sympy(xml[0])
2022-01-13T20:39:52.0966775Z   File "src/lxml/etree.pyx", line 1176, in lxml.etree._Element.__getitem__
2022-01-13T20:39:52.0967216Z IndexError: list index out of range
2022-01-13T20:39:52.0967531Z ok
2022-01-13T20:39:52.0996244Z test_symmath_check_equivalent_symbols (tests.test_symmath_check.SymmathCheckTest) ... ok
2022-01-13T20:39:52.9970904Z test_symmath_check_floats (tests.test_symmath_check.SymmathCheckTest) ... ok
2022-01-13T20:39:53.7788603Z test_symmath_check_integers (tests.test_symmath_check.SymmathCheckTest) ... ok
2022-01-13T20:39:53.7984221Z test_symmath_check_same_symbols (tests.test_symmath_check.SymmathCheckTest) ... ok
2022-01-13T20:39:53.7984503Z 
2022-01-13T20:39:53.7985274Z ----------------------------------------------------------------------
2022-01-13T20:39:53.7985902Z Ran 64 tests in 3.923s
2022-01-13T20:39:53.7986042Z 
2022-01-13T20:39:53.7986327Z OK

I've looked into this issue more. From what I can tell, this test is failing because formula.make_sympy (in formula.py) does not work correctly when interpreting xml. The recursive implementation fails when it sees a tag it is not familiar with, in this case "mstyle".

[Carlos-Muniz]

A new issue #39 has been made to add more tests that evaluate the dynamath evaluation.

[kdmccormick]

Looking pretty good; just a couple more issues.

[kdmccormick]

@Carlos-Muniz this will not fix the linting violations, but it will make so Python CI / Tests (ubuntu-20.04, 3.8, py38) runs only unit tests and Python CI / Tests (ubuntu-20.04, 3.8, quality) runs only quality checks: #40. Right now they're both running both, which is confusing.

[kdmccormick]

@Carlos-Muniz As for the new quality failures that we're seeing: no, you don't need to fix all of those, but we do need to get the build passing (without disabling quality checking altogether). Here's what I'd recommend:

For pycodestyle:

• Create a ./setup.cfg with a pycodestyle section and disable style rules that you think aren't helpful or are just beyond fixing at this point. For example, disabling E501 (line too long) will eliminate a looot of those violations.

For pylint:

• You can use edx-lint (will need to be added to this repo's testing requirements) to generate a ./pylintrc file for this repo, which will tell pylint to ignore certain violations. Similar to pycodestyle, you can choose to ignore violations that you think are unhelpful or are just too far gone to be worth fixing at this point.

[kdmccormick]

Nice work!

Two nits, but no need for another round of review IMO