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

RFC: support python 3.12 (but drop 3.8) #82

Merged
merged 8 commits into from
Sep 10, 2024
Merged

RFC: support python 3.12 (but drop 3.8) #82

merged 8 commits into from
Sep 10, 2024

Conversation

muxator
Copy link

@muxator muxator commented Feb 24, 2024

This is an experiment aimed at supporting python 3.12, which is already at its second point release.

Formally, python 3.8 is still going to be supported for 6 months (https://devguide.python.org/versions). However, some packages have already dropped it.

In particular, there is no scipy version supporting 3.8 and 3.12 at the same time, and this forces us to make a choice.

After bumping scipy to >= 1.11, poetry is able to resolve the dependencies.

At install time there were 2 more problems:

  1. numpy did not install because python 3.12 dropped support for distutils (https://peps.python.org/pep-0632/). There are workarounds for this (https://numpy.org/devdocs/reference/distutils_status_migration.html), but bumping to 1.26 seems to be an official solution
  2. statsmodels 0.13 did not compile under python 3.12, probably because it is compiled with -Werror, and there are some deprecation warnings when compiling its wheel. Bumping to 0.14 solved the issue.

This is just the easiest set of actions for allowing the project to run on python3.12. There might be ways to keep a more relaxed set of dependencies (for example, only install numpy 1.26 under python 3.12) if we want to be more permissive.

Local tests on my Fedora with python 3.12 completed fine. Opening the PR to see what the CI (and the fellow devs) think about it.

@codecov-commenter
Copy link

codecov-commenter commented Feb 24, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 69.69697% with 10 lines in your changes missing coverage. Please review.

Project coverage is 88.52%. Comparing base (c8b2cc3) to head (5906825).

Files with missing lines Patch % Lines
black_it/plot/plot_results.py 50.00% 2 Missing ⚠️
black_it/schedulers/rl/rl_scheduler.py 33.33% 2 Missing ⚠️
black_it/calibrator.py 80.00% 1 Missing ⚠️
black_it/loss_functions/base.py 66.66% 1 Missing ⚠️
black_it/samplers/halton.py 50.00% 1 Missing ⚠️
black_it/schedulers/base.py 50.00% 1 Missing ⚠️
black_it/utils/json_pandas_checkpointing.py 50.00% 1 Missing ⚠️
black_it/utils/sqlite3_checkpointing.py 50.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
- Coverage   89.22%   88.52%   -0.71%     
==========================================
  Files          42       33       -9     
  Lines        1782     1751      -31     
==========================================
- Hits         1590     1550      -40     
- Misses        192      201       +9     
Files with missing lines Coverage Δ
black_it/loss_functions/gsl_div.py 98.46% <100.00%> (-0.03%) ⬇️
black_it/loss_functions/likelihood.py 81.63% <100.00%> (ø)
black_it/samplers/xgboost.py 98.21% <100.00%> (-0.04%) ⬇️
black_it/search_space.py 98.88% <100.00%> (-0.02%) ⬇️
black_it/calibrator.py 94.08% <80.00%> (-0.60%) ⬇️
black_it/loss_functions/base.py 97.91% <66.66%> (-2.09%) ⬇️
black_it/samplers/halton.py 96.20% <50.00%> (-1.27%) ⬇️
black_it/schedulers/base.py 87.50% <50.00%> (-3.13%) ⬇️
black_it/utils/json_pandas_checkpointing.py 93.65% <50.00%> (-1.59%) ⬇️
black_it/utils/sqlite3_checkpointing.py 95.40% <50.00%> (-1.15%) ⬇️
... and 2 more

... and 30 files with indirect coverage changes

@muxator muxator force-pushed the support-3.12 branch 2 times, most recently from 156f5d2 to 554a754 Compare February 26, 2024 09:25
@AldoGl AldoGl force-pushed the support-3.12 branch 7 times, most recently from a3f0f8b to f77f4c7 Compare September 2, 2024 13:07
@AldoGl
Copy link
Contributor

AldoGl commented Sep 2, 2024

Thanks @muxator for the great work. I fixed the linting and tests, as explained also my commit, as follows:

  • updating nbmake to 1.5.4
  • adding some needed fixtures for tests
  • fixing mypy and other linting checks
  • removing checks of the "safety" library (as it is not open source anymore, they suggest to buy the paid version for more information)
  • removing test-checks that are incompatible for different OS, in particular something in the "test_main.py".

Now the PR could me merged in my opinion!

Before this change, "make safety" would complain of the following vulnerability:

-> Vulnerability found in black version 23.12.1
   Vulnerability ID: 66742
   Affected spec: <24.3.0
   ADVISORY: Affected versions of Black are vulnerable to Regular Expression Denial of Service (ReDoS) via the lines_with_leading_tabs_expanded function in the strings.py file. An attacker could exploit this vulnerability by
   crafting a malicious input that causes a denial of service.
   CVE-2024-21503
   For more information about this vulnerability, visit https://data.safetycli.com/v/66742/97c
   To ignore this vulnerability, use PyUp vulnerability id 66742 in safety’s ignore command-line argument or add the ignore to your safety policy file.
@muxator
Copy link
Author

muxator commented Sep 6, 2024

This PR will remove the dependency on Safety (https://github.com/pyupio/safety), which has become a purely commercial product for all practical purposes.

Safety < 3.0.1 has grave issues (see pyupio/safety#480) and the current version does not support performing a scan without logging into their service account (https://docs.safetycli.com/safety-docs/safety-cli-3/quick-start-guide):

You will be unable to perform vulnerability scans unless you are authenticated. Create an account and access your free trial here. If you require assistance, please email [email protected].

Let's remove the dependency on Safety altogether.

Safety (https://github.com/pyupio/safety) has become a purely commercial
platform for all practical puposes. Versions < 3.0.1 are unsupported and have
grave configurability issues. Let's remove the dependency altogether.

See: #82 (comment)
muxator pushed a commit that referenced this pull request Sep 6, 2024
Safety (https://github.com/pyupio/safety) has become a purely commercial
platform for all practical puposes. Versions < 3.0.1 are unsupported and have
grave configurability issues. Let's remove the dependency altogether.

See: #82 (comment)
@muxator muxator force-pushed the support-3.12 branch 2 times, most recently from 029c4f7 to 5906825 Compare September 6, 2024 08:45
@muxator muxator marked this pull request as draft September 6, 2024 12:32
@AldoGl AldoGl marked this pull request as ready for review September 10, 2024 08:49
@muxator muxator force-pushed the support-3.12 branch 2 times, most recently from 3b7c479 to 5906825 Compare September 10, 2024 09:50
# values on MacOS when statsmodels was updated from 0.13 to 0.14. This
# captures the change, so that when a fix arrives upstream we can update
# the test again.
BEST_PARAMETERS_STR = "Best parameters found: [0.21 0.19 0.76]"
Copy link
Contributor

Choose a reason for hiding this comment

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

very nice solution for the moment

muxator added 2 commits September 10, 2024 14:36
…12 incompatibilities

The issue with numpy is documented here, and officially solved with numpy 1.26:
https://numpy.org/devdocs/reference/distutils_status_migration.html

The upgrade on statsmodel probaby uncovers a bug in its MacOS build: the values
computed by MethodOfMomentsLoss are slightly different there, and lead to a
different result in the test.

Let's accept it for now; we can eventually file a bug on statsmodel later.
….8 due to SciPy

As of 2024-02, Python 3.8 is still going to be supproted for 6 months (until
2024-10, see https://devguide.python.org/versions).

At the same time, Python 3.12 was released four months ago, on 2023-10, and will
be supported for five years.

We cannot support both, because SciPy 1.10 does not work with python 3.12, and
scipy 1.11 drops python 3.8.

Having to make a choiche, it is probably better to go with the more recent
versions, thus 3.8 has to go down the drain.

This commit introduces some failures on the mypy check, that are going to be
fixed in the next commit.
muxator pushed a commit that referenced this pull request Sep 10, 2024
Safety (https://github.com/pyupio/safety) has become a purely commercial
platform for all practical puposes. Versions < 3.0.1 are unsupported and have
grave configurability issues. Let's remove the dependency altogether.

See: #82 (comment)
@muxator muxator force-pushed the support-3.12 branch 2 times, most recently from c2f2d82 to 5d96fb8 Compare September 10, 2024 12:54
@AldoGl
Copy link
Contributor

AldoGl commented Sep 10, 2024

Thank you @muxator and @marcofavorito for the great help for this PR. As agreed, I will rebase and merge and we can tackle the OS compatibility issue another time

@AldoGl AldoGl merged commit 8fcf978 into main Sep 10, 2024
14 checks passed
AldoGl added a commit that referenced this pull request Sep 10, 2024
Safety (https://github.com/pyupio/safety) has become a purely commercial
platform for all practical puposes. Versions < 3.0.1 are unsupported and have
grave configurability issues. Let's remove the dependency altogether.

See: #82 (comment)
@AldoGl AldoGl deleted the support-3.12 branch September 10, 2024 18:31
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.

3 participants