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: add python 3.12 support #76

Conversation

ichintanjoshi
Copy link
Contributor

Description

This PR contains changes for upgrading python 3.8 to python 3.12.

Changes include :-

  • Dependencies and version upgrades
  • Extra check for fractional function to get only integers
    • This is done because fractions function in miller.py file used fr.gcd
    • Since python 3.9: The math.gcd() function is now used to normalize the numerator and denominator. math.gcd() always return a int type. Previously, the GCD type depended on numerator and denominator
  • Removal of 3.8 from tox file

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

@openedx-webhooks
Copy link

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 11, 2024
"""
return a * b / fr.gcd(a, b)
return a * b / math.gcd(int(a), int(b))
Copy link
Contributor Author

@ichintanjoshi ichintanjoshi Mar 11, 2024

Choose a reason for hiding this comment

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

I am not sure if this is the right way to do it, can anyone else take a look into this ?

Had to change from fr.gcd to math.gcd because since python 3.9 it Fractions no longer support gcd and it has been moved to math ref

@mphilbrick211 mphilbrick211 requested a review from a team March 11, 2024 21:26
@mphilbrick211
Copy link

@openedx/2u-tnl @openedx/teaching-and-learning Hi there! Would someone be able to review this for us? Thanks!

@jristau1984
Copy link
Contributor

Hi @mphilbrick211 - T&L has relinquished ownership of this repo. Ownership updates are now reflected in the newly added "Repo Maintainer" column of the Tech Ownership sheet. Please don't hesitate to reach out with any questions, or ideas on how to better evangelize these updates. Thanks!

@@ -16,7 +16,6 @@ jobs:
matrix:
os: [ubuntu-20.04]
python-version:
- '3.8'
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't remove 3.8 testing, just add 3.12 testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so with 3.8 workflow what's happening is a conflict in numpy, because we've updated all the versions.

Something like what's happening here https://github.com/openedx/openedx-calc/actions/runs/8231433530/job/22568231165?pr=99#step:7:11

So should I downgrade numpy version and check if that'll work with both 3.8 and 3.12 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Normally you can add a version of numpy that will work with both versions to https://github.com/openedx/openedx-chem/blob/master/requirements/constraints.txt along with a comment that we can remove the constraint once the 3.8 testing is removed.

With the numpy package in particular, it may make more sense to add it here: https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt since we use it in many packages across our ecosystem.

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 current version of numpy with python 3.8 is 1.23.x here as well as in openedx/openedx-calc#99 and it's being upgraded to 1.26.4.

I manually tried to find a common version between them, python 3.8 will support till numpy==1.24.x and if we do try to install it with python 3.12, it will give AttributeError: module 'pkgutil' has no attribute 'ImpImporter'. Did you mean: 'zipimporter'? This error comes as a result of 3.12 removing ImpImporter ref or it could be because numpy depends on old pip. Also I tried to look into numpy notes but could not find an overlap with a version that'd support 3.8 and 3.12

with python 3.8, numpy maximum version supported is 1.24.4
and with python 3.12, numpy minimum version supported is 1.26.0 numpy news

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I noticed this as well in a different spot that pulls numpy. Given the importance of the package, I think we'll have to target 3.11 for edx-platform and its dependencies first and then do the 3.12 upgrade in the future.

Copy link
Contributor Author

@ichintanjoshi ichintanjoshi Mar 22, 2024

Choose a reason for hiding this comment

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

Hi @feanil okay, so should I raise another similar PR ? with 3.11 support right ? Or should I make changes here itself ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a separate PR to add testing for 3.11 and fixing any bugs that causes makes the most sense I think.

@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.

5 participants