apple / turicreate

Turi Create simplifies the development of custom machine learning models.
BSD 3-Clause "New" or "Revised" License
11.2k stars 1.14k forks source link

Published 6.4.1 requires_dist has inconsistent coremltools version #3342

Open malcolmgreaves opened 4 years ago

malcolmgreaves commented 4 years ago

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: https://github.com/python-poetry/poetry/issues/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 commented 4 years ago

I have this PR https://github.com/apple/turicreate/pull/3343, which I think will fix the publishing issue going forward.

TobyRoseman commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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.