Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: python 3.12 upgrade #99

Conversation

ichintanjoshi
Copy link
Contributor

@ichintanjoshi ichintanjoshi commented Mar 8, 2024

Description

This PR contains changes for upgrading python 3.8 to python 3.12.
Changes include :-

  • Dependencies and version upgrades
  • Extra check for factorial function to get only integers
    • This is done because evaluator function in calc.py file will convert all numbers to float
    • And python 3.12 will not allow math.factorial to calculate float values and will raise an error ref
  • Removal of float numbers for factorial function tests (Same explanation as above)

Done as a part of this issue openedx/public-engineering#233

@openedx-webhooks
Copy link

openedx-webhooks commented Mar 8, 2024

Thanks for the pull request, @ichintanjoshi! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 8, 2024
@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 8, 2024
@ichintanjoshi ichintanjoshi marked this pull request as ready for review March 11, 2024 10:42
@@ -348,9 +348,7 @@ def test_other_functions(self):
self.assert_function_values('factorial', fact_inputs, fact_values)

self.assertRaises(ValueError, calc.evaluator, {}, {}, "fact(-1)")
self.assertRaises(ValueError, calc.evaluator, {}, {}, "fact(0.5)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were these assertions removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@feanil python 3.12 will not allow math.factorial to calculate float values and will raise an error ref

So earlier if we sent integers like 1.0 it'll work but if we sent float 1.5 it used to give value error
but now if we send integers like 1.0 it'll give error, and because of that I wrote extra code to check if it's factorial we'll convert all incoming numbers to integers, because calc.py has evaluator function which converts all numbers to floats. And if we send it 0.5 it'll just take it as 0 and the test won't work..

Copy link
Contributor

@feanil feanil Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ichintanjoshi Right, but I'm confused, it sounds like you're changing the behavior. This test confirms that it does raise a value error which is what we want. math.factorial raised a value error in 3.8 as well so we shouldn't need to change anything here. Can you share the output of the failing test that you were fixing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@feanil, yes,

I'll try to explain my changes.

Python 3.8

The value we were sending from here test_calc.py

Were being sent as 0.0 or 1.0 5.0 etc and that was allowed to be calculated in python 3.8 factorial.

Python 3.12

While the values being sent are still the same i.e. 0.0 or 1.0, it's no longer allowed in python 3.12 to pass them like that in float, we need to pass them as 0 or 1 like integers, as a result I made this change, to convert these values to integer if the function being called is integer change ref

What happens now is 0.5 will be parsed as 0, because I am now converting things passed for factorials as integers. Thus I removed it.

As for the error output if we don't make changes, it is something like this.

======================================================================
ERROR: test_other_functions (tests.test_calc.EvaluatorTest.test_other_functions)
Test the non-trig functions provided in calc.py
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/chintan/openedx-repos/openedx-calc_old/tests/test_calc.py", line 347, in test_other_functions
    self.assert_function_values('fact', fact_inputs, fact_values)
  File "/home/chintan/openedx-repos/openedx-calc_old/tests/test_calc.py", line 168, in assert_function_values
    result = calc.evaluator({}, {}, input_str)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/chintan/openedx-repos/openedx-calc_old/calc/calc.py", line 281, in evaluator
    return math_interpreter.reduce_tree(evaluate_actions)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/chintan/openedx-repos/openedx-calc_old/calc/calc.py", line 454, in reduce_tree
    return handle_node(self.tree)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/chintan/openedx-repos/openedx-calc_old/calc/calc.py", line 450, in handle_node
    handled_kids = [handle_node(k) for k in node]
                    ^^^^^^^^^^^^^^
  File "/home/chintan/openedx-repos/openedx-calc_old/calc/calc.py", line 450, in handle_node
    handled_kids = [handle_node(k) for k in node]
                    ^^^^^^^^^^^^^^
  File "/home/chintan/openedx-repos/openedx-calc_old/calc/calc.py", line 450, in handle_node
    handled_kids = [handle_node(k) for k in node]
                    ^^^^^^^^^^^^^^
  [Previous line repeated 2 more times]
  File "/home/chintan/openedx-repos/openedx-calc_old/calc/calc.py", line 451, in handle_node
    return action(handled_kids)
           ^^^^^^^^^^^^^^^^^^^^
  File "/home/chintan/openedx-repos/openedx-calc_old/calc/calc.py", line 268, in eval_function
    return all_functions[casify(x[0])](x[1])
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: 'float' object cannot be interpreted as an integer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ichintanjoshi I dug into it a little deeper, based on your stack trace, it looks like the error is happening with the call on line 347 and not on the line that you removed. I don't think the fix you put in place is the right one because we don't want to coerce actual floats to integers as that will fail to raise errors when it correctly should.

I think the real problem is with the eval_number function which coerces all numbers to floats. I think that function needs to be smarter and if there are no decimals in the number then it needs to return integers instead of numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was the issue, Okay I'll take a look into it if I can fix it.

@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 13, 2024
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ichintanjoshi so it looks like there is no version of numpy that supports both Python 3.8 and 3.12 so for edx-platform and its librararies(including this one) we should aim to get it working on 3.8 and 3.11 instead. For the dependencies, we should still compile them with 3.8 but run the test with both 3.8 and 3.11 so that we can be confident that it will work with both.

@@ -348,9 +348,7 @@ def test_other_functions(self):
self.assert_function_values('factorial', fact_inputs, fact_values)

self.assertRaises(ValueError, calc.evaluator, {}, {}, "fact(-1)")
self.assertRaises(ValueError, calc.evaluator, {}, {}, "fact(0.5)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ichintanjoshi I dug into it a little deeper, based on your stack trace, it looks like the error is happening with the call on line 347 and not on the line that you removed. I don't think the fix you put in place is the right one because we don't want to coerce actual floats to integers as that will fail to raise errors when it correctly should.

I think the real problem is with the eval_number function which coerces all numbers to floats. I think that function needs to be smarter and if there are no decimals in the number then it needs to return integers instead of numbers.

@ichintanjoshi
Copy link
Contributor Author

Hi @feanil I checked it's not through eval_number, I did make it to return int only, but that fails with the same error. In the same place, I think the number is getting it's float value from somewhere else.

For example here is the code that I tried it with

def eval_number(parse_result):
    """
    Create a float out of its string parts.

    e.g. [ '7.13', 'e', '3' ] ->  7130
    Calls super_float above.

    or return int, if numbers are int only
    """
    joined_result = "".join(parse_result)
    try:
        if isinstance(eval(joined_result), numbers.Number):
            if math.ceil(eval(joined_result)) == eval(joined_result):
                return math.ceil(eval(joined_result))
    except Exception as e:
        pass


    return super_float(joined_result)

@openedx-webhooks
Copy link

@ichintanjoshi Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants