Switch to Poetry for dependency management and package description #356
Conversation
There was a problem hiding this comment.
Looks good to me. We've started to (partly) use Poetry for https://github.com/MIT-LCP/physionet-build so we have some familiarity with it.
I've added a couple of minor comments (+ tests look like they are failing, though haven't checked why).
|
I assume you have to specify the python version in https://github.com/MIT-LCP/wfdb-python/blob/master/.github/workflows/run-tests.yml for the ubuntu-latest build to get the tests to pass (currently it looks like it wants to install Python 3.1). @bemoody set up this test so we should double check he's okay with dropping the older Python versions here. |
|
I'm very much in favor of adding test cases for newer systems and newer external packages. I'm very much opposed to dropping test cases or breaking compatibility for no reason. This package shouldn't be in the business of requiring or even recommending specific external package versions. You should use whatever version of numpy is available for your system, and this package should be compatible with as wide a range of numpy versions as is reasonably possible. Removing Removing setup.py sounds like a pretty bad idea too. |
|
Restored the Python 3.7 version compatibility. Found a set of dependencies which work for Python 3.7-3.10, which also include scipy 1.7 for M1 macs: scipy/scipy#13409 My original justification for removing support for P3.7 was due to me misreading its EOL date as June 2022 instead of 2023. |
So overall, I agree that we should try to support as many different platforms as possible, as long as they do not impair the development of this package. Removing P37 was a mistake. I spent a good amount of time playing around with dependency versions, and have finally found a compatible set which supports all the LTS OSs, in addition to newly supporting the M1 macs due to the scipy issue. The main tradeoff is removing P36, which is already past its end of life, and isn't used by any of the LTS OSs anymore. I think that's a very fair item to remove. Regarding the rest of your comment, there are several important points to be considered: This package has always had dependencies, as specified in Poetry has forced the package to explicitly set a more restrictive dependency (independent of me increasing the dependency versions), by highlighting the restrictions which were always there. They were previously not shown because we had no way of testing them. Adding a larger testing matrix and running our sparse unit tests (which is very useful and I fully support independenly,) in order to figure out compatibility is vastly inferior to having poetry deterministically figuring out compatibility. If our dependencies all have their own dependency graphs and limitations, and we choose to ignore them, as long as our unit tests pass, we're not doing our users any favors. Leaving our users to fiddle around with trying to install a compatible dependency range, while not telling them the true set of restrictions, is not useful. This package is being no more restrictive than its dependencies. And these changes are just making the dependencies explicit, where they were previously hidden. I see this similar to catching errors at compile-time rather than run-time. Finally, I'll say that there is a tradeoff in keeping a wider compatibility matrix. An exaggerated example would be to support Python 2 and 3. There's much overhead in terms of development time, which at some point, just isn't worth it. Especially because there are so few of us active contributors, we need to be careful about how much time we dedicate to this.
Restored. I wanted to rely on another method for getting the version info but that's only supported for P3.8.
It's redundant with poetry and
I don't see a reason to keep it. See, for example, the fastapi repo, which doesn't have one. I imagine you're not against using poetry itself. If so, that's an entirely different discussion. |
I was confused by this: apparently pip can use the information in pyproject.toml as a substitute for setup.py. That seems quite reasonable. I don't understand why pyproject.toml mentions "tool.poetry" if this file is not related to poetry, but that's okay. |
I haven't used poetry so I don't know what it does or doesn't force you to do. If it forces you to make your own software less forwards- or backwards-compatible then that's terrible design and a good reason not to use it. But I doubt it actually forces you to do that. I'll be specific: wfdb-python is known to work with these:
I don't see any reason to suspect that wfdb-python will not work with these, once they are released:
Preemptively declaring that any unsupported/untested version is incompatible is a nightmare, both for people in the future and for people in the present who want to use your library in combination with anything else. A separate question to what external package versions are compatible is whether you want to ship, in this repository, a massive list of hashes for other packages. I think that's not a great idea because that list will always be incomplete and out of date. I'm not inclined to go to any effort to maintain such a list and certainly not to test or audit those packages. But I suppose you can argue the list doesn't hurt if people aren't required to use it. |
|
Can you clarify what you mean by "deterministically figuring out compatibility"? Maybe there is some point there I have missed. If there is some automated mechanism that's telling you this package isn't compatible with numpy 1.16, that would be really interesting (even if I don't believe it.) If it's telling you numpy 1.16 isn't officially supported or officially developed, that's also an interesting piece of information but not relevant in any technical sense. (If it's telling you it's not possible to prove, on the basis of what you currently have installed, that numpy 1.16 is compatible, that's very different from telling you numpy 1.16 is incompatible.) If someone is telling you that this package is not going to be compatible with requests 2.28, their crystal ball needs adjusting. |
Yes, there's an automated mechanism that tells you if a set of dependencies you list clashes. Including Python versions. Try it out and see. What I'll do is revert the dependencies to their existing values, and try to upgrade them in a separate PR so that the two issues are not confounded. Will archive this set here: [tool.poetry.dependencies]
python = ">=3.7.1,<3.11"
numpy = ">=1.19.0"
pandas = "~1.3.5"
requests = "~2.27.1"
scipy = {version = "~1.7.0", optional = true}
matplotlib = {version = "~3.5.1", optional = true}
[tool.poetry.extras]
plot = ["matplotlib"]
processing = ["scipy"] |
|
Ok, I've converted all the tilde dependency ranges to use caret for more flexibility. All dependency versions in Python is ^3.6.1 because Pandas will not allow it to be ^3.6 Let me know if this dependency set is acceptable. I'm going to try to fix the actions CI. |
|
Yeah that makes sense. Thanks!
|
|
It looks like according to: https://github.com/MIT-LCP/wfdb-python/actions/runs/2185363249 that we are having issues with Python 3.6 (which is past its end of life) and 3.10. The 3.10 error only comes from nosetests. I tried running Pytest on a 3.10 container and all the tests pass. Here's what I suggest:
|
| run: | | ||
| nosetests | ||
| python -m pip install --upgrade pip poetry | ||
| pip install ".[dev]" |
There was a problem hiding this comment.
We're using pip to install the dependencies instead of poetry for a few reasons:
- We want to support pip installations. This will probably be the primary way for most users.
- A deployment doesn't actually need poetry, which would add another dependency. Use poetry primarily for developing.
- Poetry has its own system of managing environments, and where packages are installed by default, which can get complex. https://realpython.com/dependency-management-python-poetry/ eg. When trying to run the CI earlier with poetry,
poetry installwould install the packages (including the test executable) into~/.cachewhich weren't automatically usable by the system installed Python (causing all those test failures). We don't need to deal with this.
Both sound fine to me. Often you can do a straight swap with pytest. Nosetests is old and should be replaced regardless. |
|
A few things:
I think it would be fine (as a first step) to limit the github workflows to python 3.7 through 3.9, but pyproject.toml should not declare the package as being incompatible with python 3.10, because it isn't. I don't have a strong feeling about support for python 3.6, but bear in mind that Ubuntu 18.04 is still in its "general support" period for another year. |
Time to switch to the newer tool used by many projects including scipy and fastapi.
https://python-poetry.org/docs/basic-usage/
Benefits:
setup.py,MANIFEST.in,requirements.txt,version.py(to be discussed) andtwine.requirements.txtin sync withsetup.py. When you runpoetry add <package>, it'll update bothpyproject.tomlandpoetry.lock.This is also in preparation of adding more dev dependencies: pylint and blank.
Speaking of the dependency clash, this PR also updates a few project dependencies. Most notable are:
3.83.7-3.10.Python 3.7 is already near end of life: https://www.python.org/downloads/Specifying dep versions: https://python-poetry.org/docs/dependency-specification/