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

[BugFix] python version #204

Merged
merged 3 commits into from
Aug 8, 2024
Merged

Conversation

ShuN6211
Copy link
Contributor

@ShuN6211 ShuN6211 commented Aug 2, 2024

Enable to run "poetry install".

Description

Update pyproject.toml to be enable to run poetry install and install rl4co project by poetry tool.
At current project, running poetry install may causes following errors.

The current project's Python requirement (>=3.8) is not compatible with some of the required packages Python requirement:
  - pyvrp requires Python <4.0,>=3.9, so it will not be satisfied for Python >=3.8,<3.9 || >=4.0
  - pyvrp requires Python <4.0,>=3.9, so it will not be satisfied for Python >=3.8,<3.9 || >=4.0
  - pyvrp requires Python <4.0,>=3.9, so it will not be satisfied for Python >=3.8,<3.9 || >=4.0
Because no versions of pyvrp match >0.8.2,<0.9.0 || >0.9.0,<0.9.1 || >0.9.1
 and pyvrp (0.8.2) requires Python <4.0,>=3.9, pyvrp is forbidden.
And because pyvrp (0.9.0) requires Python <4.0,>=3.9, pyvrp is forbidden.
So, because pyvrp (0.9.1) requires Python <4.0,>=3.9
 and rl4co depends on pyvrp (>=0.8.2), version solving failed.
  • Check your dependencies Python requirement: The Python requirement can be specified via the `python` or `markers` properties
    For pyvrp, a possible solution would be to set the `python` property to ">=3.9,<4.0"
    For pyvrp, a possible solution would be to set the `python` property to ">=3.9,<4.0"
    For pyvrp, a possible solution would be to set the `python` property to ">=3.9,<4.0"

Motivation and Context

  • I have raised an issue to propose this change (required for new features and bug fixes)

Types of changes

What types of changes does your code introduce? Remove all that do not apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds core functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (update in the documentation)
  • Example (update in the folder of examples)

Checklist

Go over all the following points, and put an x in all the boxes that apply.
If you are unsure about any of these, don't hesitate to ask. We are here to help!

  • My change requires a change to the documentation.
  • I have updated the tests accordingly (required for a bug fix or a new feature).
  • I have updated the documentation accordingly.

Enable to run "poetry install".
@fedebotu
Copy link
Member

fedebotu commented Aug 2, 2024

Hi @ShuN6211 , thanks for reporting this!

How about this instead:

pyvrp = { version = ">=0.8.2", optional = true, python = ">=3.9,<4.0" }

in the dependencies. This is because pyvrp is optional, and we would like not to constrain rl4co too much based on such dependencies. I tried poetry install, and it works ~

@ShuN6211
Copy link
Contributor Author

ShuN6211 commented Aug 3, 2024

Hi @fedebotu , thanks for commenting!!
It's a good idea to specify python version "<4.0" not in dependencies but in optional scope. 😃
However, only changing a line regarding "pyvrp" still causes another error in my environment.
It seems numba(optional) and lightning requires the numpy that requires python = ">=3.9"
I tried following chunk and it works.

⭕️ Works

[tool.poetry.dependencies]
# Required dependencies
python = ">=3.9"
# Routing
pyvrp = { version = ">=0.8.2", optional = true, python = "<4.0" }

❌ Doesn't work , still causes another error

[tool.poetry.dependencies]
# Required dependencies
python = ">=3.8"
# Routing
pyvrp = { version = ">=0.8.2", optional = true, python = ">=3.9,<4.0" }

Procedure

  • Create some venv. ( I use miniconda to create venv)
  • Activate venv
  • python -m pip install poetry
  • rm poetry.lock if there is
  • poetry install

Side notes

  • machine: M1 macbook Pro 2021, macOS Ventura13.4.1
  • python version: 3.10
  • poetry version: 1.5.0

@fedebotu
Copy link
Member

fedebotu commented Aug 4, 2024

I see! This is better, in my opinion, since we may want to restrict a lower version of Python rather than the upper one (Python 3.8 is pretty old anyway). To be honest, though, I'm not a fan of restricting the main Python version because of some optional dependency, so if there was another way, I'd rather go for that :)

Thoughts @cbhua @LTluttmann?

@fedebotu
Copy link
Member

fedebotu commented Aug 7, 2024

Update: the newest PyTorch Lightning version has been released, and dropped support for Python 3.8 too:
https://github.com/Lightning-AI/pytorch-lightning/releases/tag/2.4.0

Given this seems to be standard practice by now, I'd say let's remove official support for 3.8 altogether and follow @ShuN6211 's suggestion

@ShuN6211
Copy link
Contributor Author

ShuN6211 commented Aug 8, 2024

@fedebotu
Thanks for sharing the dropping support for python 3.8 in the newest PyTorch Lightning.
I added the revert commit and a new one that reflects conversation between us.
If you don't prefer revert, I would reopen new PR.

@fedebotu fedebotu requested a review from cbhua August 8, 2024 03:37
@fedebotu fedebotu added this to the 0.5.0 milestone Aug 8, 2024
@cbhua
Copy link
Member

cbhua commented Aug 8, 2024

@ShuN6211 Thanks for reporting the bug about the Python version!

Considering the status of Python versions from the official release cycle that Python 3.8's end-of-life day is 2024-10, I agree that let's stop the support for Python 3.8.

@fedebotu fedebotu merged commit 4170ef1 into ai4co:main Aug 8, 2024
14 checks passed
@ShuN6211 ShuN6211 deleted the hotfix-python_version branch August 8, 2024 08:07
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