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

Update requirements in preparation for 3.12 #271

Closed
wants to merge 10 commits into from

Conversation

CRD716
Copy link
Contributor

@CRD716 CRD716 commented Oct 23, 2023

Prepare for 3.12, fix things, etc etc, (inb4 this ruins everything)

I have specifically only updated to versions with no breaking changes, in some cases (ex. urllib3) the version I have selected is extremely out of date, but I wanted to be conservative in my updating.

Links to all not fully updated dependencies' changelogs, ordered by how out of date they still are:

  1. urllib3 https://github.com/urllib3/urllib3/blob/main/CHANGES.rst (Updating to 1.26.7, latest is 2.0.7, many things have changed.)
  2. yaspin https://github.com/pavdmyt/yaspin/releases (Updating to 2.5.0, latest is 3.0.1, small change list, worth investigating.)
  3. peewee https://github.com/coleifer/peewee/releases (Updating to 3.16.3, latest is 3.17.0, seems like behavior has changed.)

Based on some of the changelogs I've read, this could also increase performance in some areas.

Prepare for 3.12, fix things, etc etc, inb4 this ruins everything
@CRD716
Copy link
Contributor Author

CRD716 commented Oct 23, 2023

questionary 2.0.1 depends on prompt_toolkit<=3.0.36 and >=2.0, whoops.

@CRD716
Copy link
Contributor Author

CRD716 commented Oct 23, 2023

The regression in prompt_toolkit can be traced to tmbo/questionary#270.

Explicitly set the prompt_toolkit version to be <=3.0.36 because 3.0.37 has introduced some event-loop-related bug.

@Umpire2018
Copy link
Contributor

First and foremost, thank you very much for your contribution.

Speaking of version changes, I suggest introducing modifications to pilot/utils/utils.py in this PR. The current version used by distro is 1.8.0. The usage of distro.linux_distribution in line 119 was deprecated in version 1.6.0. I recommend making changes. @CRD716

Also, considering dependency management is a tricky issue, I recommend introducing a dependency management tool like Poetry to handle it. For instance, using poetry update can update the versions of the packages being used.

@LeonOstrez @nalbion

 distro.name(pretty=False)

    Return the name of the current OS distribution, as a human-readable string.

    If pretty is false, the name is returned without version or codename. (e.g. “CentOS Linux”)

    If pretty is true, the version and codename are appended. (e.g. “CentOS Linux 7.1.1503 (Core)”)
@CRD716
Copy link
Contributor Author

CRD716 commented Oct 23, 2023

Speaking of version changes, I suggest introducing modifications to pilot/utils/utils.py in this PR. The current version used by distro is 1.8.0. The usage of distro.linux_distribution in line 119 was deprecated in version 1.6.0. I recommend making changes. @CRD716

Should now match current recommendations.

@MRigal
Copy link

MRigal commented Oct 30, 2023

You may also want to update the README.md to remove the hint to the psycopg2 binaries for Windows, which are now fixed and supported.

Also the tests for Linux should be able to run again

@CRD716
Copy link
Contributor Author

CRD716 commented Oct 30, 2023

Same test failure as main, not pr related.

@CRD716
Copy link
Contributor Author

CRD716 commented Oct 31, 2023

@nalbion ready for testing/merge 👍

Copy link
Contributor

@senko senko left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! 🙏

Current main works with 3.12 (as psycopg2 was upgraded in another PR). I agree it would be good to update the other packages as well (what this PR does), except maybe bumping the major version for questionary.

I've added a few comments inline.

pilot/utils/utils.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
@CRD716
Copy link
Contributor Author

CRD716 commented Nov 13, 2023

@senko Updated as per your comments 👍

@CRD716 CRD716 requested a review from senko November 15, 2023 03:45
@senko
Copy link
Contributor

senko commented Nov 19, 2023

Hey @CRD716 I've just rebased and merged your changes in 36e5d6a. There were some merge conflicts which I resolved.

We've also switched to working in development branch and periodically merging to main after additional testing (to minimize breaking stuff for people), so I rebased on top of that one and merged to it.

Thanks for your contribution! 🙏

@senko senko closed this Nov 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants