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 #24

Conversation

ichintanjoshi
Copy link

@ichintanjoshi ichintanjoshi commented Feb 29, 2024

Description

This PR contains changes for upgrading python 3.8 to python 3.12 while also supporting 3.11.

Changes include dependencies version upgrades

Done as a part of following :-

@openedx-webhooks
Copy link

openedx-webhooks commented Feb 29, 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 Feb 29, 2024
@mphilbrick211
Copy link

Hi @ichintanjoshi! Thanks for this contribution! Please let me know if you have any questions regarding submitting a CLA form.

@ichintanjoshi
Copy link
Author

Hi @mphilbrick211 I've submitted a CLA yesterday, let me know if there is anything else to be done from my side.

@ichintanjoshi
Copy link
Author

Hi @mphilbrick211 CLA process is done.

catalog-info.yaml Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@ichintanjoshi ichintanjoshi force-pushed the jenkins/add-python312-support-95d6614 branch from bf60a80 to afab50c Compare March 5, 2024 12:21
tox.ini Outdated
@@ -14,7 +14,8 @@ commands = flake8 user_util
[testenv]
setenv =
PYTHONPATH = {toxinidir}
deps =
deps =
setuptools
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than doing this here, I think it should be an updated in the relevant requirements/*.in files.

We should definitely add it to the tox.in file which is what's causing the package build to fail.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I'll add and check it accordingly

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it looks like part of the issue might be the really old version of tox. It may be worth running make upgrade manually in this repo to get the dependencies up to dot. It may be worth doing that in a separate PR. That might fix it without requiring setuptools manually in tox.in

Copy link
Author

Choose a reason for hiding this comment

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

After removing it from here and adding it in tox.in, if I check the run in nektos act, it still gives the same error

Copy link
Author

@ichintanjoshi ichintanjoshi Mar 6, 2024

Choose a reason for hiding this comment

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

@feanil after running make upgrade other dependencies are now updated and adding setuptools back to this file the tests are now passing in the act. If I don't keep it in this file it still fails

I took reference from here

@ichintanjoshi ichintanjoshi marked this pull request as ready for review March 7, 2024 04:16
@ichintanjoshi ichintanjoshi changed the title python 3.12 upgrade feat: python 3.12 upgrade Mar 7, 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.

As I've done a couple of these, I've realized that we should still be building the requirements using Python 3.8 especially in libraries so that we don't test with newer version of dependencies than an upstream running python 3.8 can handle. We want to be able to run with python 3.8 and 3.11/3.12 using 3.8 dependencies. Can we update this to use 3.8 dependencies?

@ichintanjoshi
Copy link
Author

Hi @feanil Okay so I need to create a venv with 3.8 and run make upgrade command ?

Or is it something else ?

@ichintanjoshi
Copy link
Author

@feanil this one passes all the tests with 3.12 and doesn't have numpy as well. Should I keep this PR open or close it and create a new one with 3.11 ?

@feanil
Copy link
Contributor

feanil commented Mar 26, 2024

I think you can keep it and just add testing for 3.11 as well, end yes, the make upgrade should still be run with Python 3.8 for now.

@ichintanjoshi
Copy link
Author

@feanil added 3.11 part too.

@mphilbrick211
Copy link

@feanil - would you mind re-enabling tests to run?

requirements/base.txt Outdated Show resolved Hide resolved
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.

Generally looks good, can you rebase and resolve conflicts and bump the minor version. Then I think this will be good to go.

@@ -1,3 +1,3 @@
"""Top-level package for Open edX User Utilities."""

__version__ = '1.0.0'
__version__ = '1.0.1'
Copy link
Contributor

Choose a reason for hiding this comment

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

We should do at least a minor version bump.

Suggested change
__version__ = '1.0.1'
__version__ = '1.1.0'

@ichintanjoshi ichintanjoshi force-pushed the jenkins/add-python312-support-95d6614 branch from 85d1f0b to b4c7372 Compare April 8, 2024 15:12
@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.

1 similar comment
@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