Skip to content
This repository has been archived by the owner on Dec 21, 2023. It is now read-only.

Published 6.4.1 requires_dist has inconsistent coremltools version #3342

Open
malcolmgreaves opened this issue Oct 2, 2020 · 6 comments · May be fixed by #3343
Open

Published 6.4.1 requires_dist has inconsistent coremltools version #3342

malcolmgreaves opened this issue Oct 2, 2020 · 6 comments · May be fixed by #3343

Comments

@malcolmgreaves
Copy link

The PyPi published package for turicreate version 6.4.1 has inconsistent dependency information.

The package's requires_dist from pypi/turicreate/json has "coremltools (==3.3)". (From curl -sL https://pypi.org/pypi/turicreate/json | jq '.info.requires_dist').

However, the wheel's requires_dist is different: executing $ pkginfo requires_distturicreate-6.4.1-cp38-cp38-manylinux1_x86_64.whl lists the dependency as 'coremltools (==4.0b3)'.

This means that installation of turicreate with tools that use the published requires_dist will fail for Python 3.8 environments. For instance, I ran into this when using poetry: python-poetry/poetry#3011.

Given that setup.py puts the version at 4.0b3 for Python 3.8 and 3.3 for Python 3.7, I believe that the install_requires in setup.py should specify the compatible versions explicitly.

@malcolmgreaves
Copy link
Author

I have this PR #3343, which I think will fix the publishing issue going forward.

@TobyRoseman
Copy link
Collaborator

Thanks for the information and the pull request.

So to summarize: in our setup.py we need to put all version requirements in install_requires, not just the requirements for that particular wheel. Is that correct?

I think the coremltools difference may just be the first issue you're going to hit here. Looking at setup.py there are a few other dependencies that will be different for different wheels.

@malcolmgreaves
Copy link
Author

Hi @TobyRoseman thanks for the assistance!

I believe that is the right course of action, to have the install_requires specify all dependencies alongside Python version requirements, if applicable. (I'm no Python packaging expert -- I got the PR solution from python-poetry/poetry#3011 and some googling).

@malcolmgreaves
Copy link
Author

malcolmgreaves commented Oct 2, 2020

If you want, I can add the other python version requirement strings to the PR I have out. I'd need them anyway 😅 so I can use turicreate with a poetry project 😄

@abn
Copy link

abn commented Oct 2, 2020

Adding some context since @malcolmgreaves mentioned the poetry thread. For tools like poetry, because we do dependency resolution ahead of time, we rely on metadata first (when available). This means that the information served from PyPI is used first. If one is not available, we do inspect the platform wheels to identify the metadata.

Typically, in a best practice scenario, one would expect PEP 508 Environment Markers to be relied upon to provide consistent metadata across all wheels. Unfortunately, setuptool based concention has most often been to rely upon conditional logic within setup.py, which leads us to the current situation of partial metadata.

From a personaly opinion perspective, it would be great if projects did this the best they can. For example your install_requires and extras_require could be something like this.

install_requires = [
    "Pillow>=5.2.0",
    "PrettyTable==0.7.2",
    "decorator>=4.0.9",
    "numpy>=1.19.2,<2.0.0",
    "pandas>=0.23.2",
    "requests>=2.9.1",
    "resampy==0.2.1",
    "scipy>=1.1.0",
    "six>=1.10.0",
]

extras_require = {
    ':python_version < "3.8"': ["coremltools==3.3"],
    ':python_version >= "2.7" and python_version < "2.8" or python_version >= "3.5" and python_version < "3.6"': [
        "llvmlite==0.31.0"
    ],
    ':python_version >= "3.8.0" and python_version < "4.0.0"': ["coremltools==4.0b3"],
    ':sys_platform != "darwin"': [
        "tensorflow>=2.0.0,<2.1.0",
        "tensorflow>=2.0.0",
        "numba<0.51.0",
    ],
    ':sys_platform == "darwin"': ["tensorflow>=2.0.0"],
}

The generated PKG-INFO will look something like this.

Requires-Dist: Pillow (>=5.2.0)
Requires-Dist: PrettyTable (==0.7.2)
Requires-Dist: coremltools (==3.3); python_version < "3.8"
Requires-Dist: coremltools (==4.0b3); python_version >= "3.8.0" and python_version < "4.0.0"
Requires-Dist: decorator (>=4.0.9)
Requires-Dist: llvmlite (==0.31.0); python_version >= "2.7" and python_version < "2.8" or python_version >= "3.5" and python_version < "3.6"
Requires-Dist: numba (<0.51.0); sys_platform != "darwin"
Requires-Dist: numpy (>=1.19.2,<2.0.0)
Requires-Dist: pandas (>=0.23.2)
Requires-Dist: requests (>=2.9.1)
Requires-Dist: resampy (==0.2.1)
Requires-Dist: scipy (>=1.1.0)
Requires-Dist: six (>=1.10.0)
Requires-Dist: tensorflow (>=2.0.0); sys_platform != "darwin"
Requires-Dist: tensorflow (>=2.0.0); sys_platform == "darwin"
Requires-Dist: tensorflow (>=2.0.0,<2.1.0); sys_platform != "darwin"

You can also just put this into your install_requires.

This was generated from the following. Obviously, I am not familiar with this project or how the dependencies are used. But hopefully this is useful.

[tool.poetry.dependencies]
python = "^3.8"
decorator = ">=4.0.9"
numpy = "^1.19.2"
pandas = ">=0.23.2"
Pillow = ">=5.2.0"
PrettyTable = "0.7.2"
resampy = "0.2.1"
requests = ">=2.9.1"
scipy = ">=1.1.0"
six = ">=1.10.0"
llvmlite = { version = "0.31.0", python = "~2.7 || ~3.5" }
coremltools = [
    { version = "4.0b3", python = "^3.8.0" },
    { version = "3.3", python = "<3.8" }
]
tensorflow = [
    { version = ">=2.0.0", markers = "sys_platform == 'darwin'" },
    { version = ">=2.0.0,<2.1.0",  python = "~2.7 || <3.8", markers = "sys_platform != 'darwin'" },
    { version = ">=2.0.0",  python = "^3.8.0", markers = "sys_platform != 'darwin'" }
]
numba = { version = "<0.51.0", markers = "sys_platform != 'darwin'" }

@Ivoz
Copy link

Ivoz commented Jan 29, 2021

Additionally, it might be possible to simplify this if your requirements for the coremltools package is simpler now. maybe you just require 4.0, or 3.3 and above will work fine on both python 3.7 and 3.8, but somehow 4.0b3 was specified accidentally (or accidentally automatically by a tool).

I can't see any obvious changelog entries in coremltools relating to it not working with any particular recent pythons.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants