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

Support py3.10 and remove py3.7 #370

Merged
merged 13 commits into from
May 3, 2023

Conversation

Mdnaimulislam
Copy link
Collaborator

@Mdnaimulislam Mdnaimulislam commented Apr 28, 2023

Fixes #368.

Description

Added new python version 3.10 and removed python version 3.7.

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • In-line docstrings updated.
  • Source for documentation at docs manually updated for new API.

@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2023

Codecov Report

Merging #370 (8cf9573) into main (28964f0) will increase coverage by 0.00%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main     #370   +/-   ##
=======================================
  Coverage   91.52%   91.52%           
=======================================
  Files          49       49           
  Lines        5226     5228    +2     
=======================================
+ Hits         4783     4785    +2     
  Misses        443      443           

see 2 files with indirect coverage changes

@xianyuanliu
Copy link
Member

It seems that Python 3.10 costs much more time in dependency installation than 3.8 and 3.9.

@Mdnaimulislam Mdnaimulislam added work-in-progress Work in progress that should NOT be merged bug Something isn't working labels May 2, 2023
Copy link
Collaborator Author

@Mdnaimulislam Mdnaimulislam left a comment

Choose a reason for hiding this comment

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

It looks like the sklearn package (not supported by Python 3.10) is being used by one of our dependencies. See this https://github.com/scikit-learn/sklearn-pypi-package, where it says to track down which dependency is using sklearn. I have tried to find it for the last couple of days but haven't been able to track it down. So, for the time being, I have set the environment variable SKLEARN_ALLOW_DEPRECATED_SKLEARN_PACKAGE_INSTALL=True to avoid this error (as provided by the above URL I mentioned).

Copy link
Member

@xianyuanliu xianyuanliu left a comment

Choose a reason for hiding this comment

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

Thanks and well done! There are some minor enhancements. This bypassing may be tracked, and removed once it becomes unnecessary.

.github/workflows/test.yml Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@Mdnaimulislam Mdnaimulislam removed the work-in-progress Work in progress that should NOT be merged label May 2, 2023
@xianyuanliu
Copy link
Member

It looks like the sklearn package (not supported by Python 3.10) is being used by one of our dependencies. See this https://github.com/scikit-learn/sklearn-pypi-package, where it says to track down which dependency is using sklearn. I have tried to find it for the last couple of days but haven't been able to track it down. So, for the time being, I have set the environment variable SKLEARN_ALLOW_DEPRECATED_SKLEARN_PACKAGE_INSTALL=True to avoid this error (as provided by the above URL I mentioned).

I may find one of the reasons. Installing nilearn-0.6.2 will raise this error, but nilearn>=0.7.0 won't. In the successful test with Python 3.10, only nilearn-0.10.1 is installed, so everything is fine. But in the failed test, 0.6.2 to 0.10.1 are installed . A pip upgrade causes installing all versions of nilearn in some situations, but I have not understood it yet. If you have any knowledge about it, please let me know. Thanks @Mdnaimulislam.

Considering that bypassing may also bypass other issues, I suggest fixing nilearn here to version nilearn>=0.7.0 and testing whether this error is resolved.

This installation error can be reproduced by the following codes.
SKLEARN_ALLOW_DEPRECATED_SKLEARN_PACKAGE_INSTALL=False pip install nilearn==0.6.2
SKLEARN_ALLOW_DEPRECATED_SKLEARN_PACKAGE_INSTALL=False pip install nilearn==0.7.0

@Mdnaimulislam Mdnaimulislam added the work-in-progress Work in progress that should NOT be merged label May 2, 2023
@Mdnaimulislam
Copy link
Collaborator Author

I didn't have any idea that the error was being caused by the nilearn version. After my research, my assumption was that it was due to the setuptools version. I encountered a couple of places where the same problem arose because of setuptools. So yes, this problem is related to pip and setuptools. When I was trying to find the cause locally, I came across an error: "SetuptoolsDeprecation: setuptools.installer is deprecated. Requirements should be satisfied by a PEP 517 installer." I am not entirely sure if this is connected to the problem we are having. I have changed the nilearn version; let's see if this solves our problem.

@Mdnaimulislam Mdnaimulislam removed the work-in-progress Work in progress that should NOT be merged label May 2, 2023
@xianyuanliu xianyuanliu self-requested a review May 2, 2023 15:45
@xianyuanliu
Copy link
Member

There are still some Python 3.7 usages in workflow and description. We'd better update them.

Copy link
Member

@xianyuanliu xianyuanliu left a comment

Choose a reason for hiding this comment

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

LGTM

@xianyuanliu xianyuanliu merged commit 92bec04 into pykale:main May 3, 2023
7 checks passed
This was referenced Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug]got error while running tutorial
4 participants