Reduce manual pre-installation requirements via pyproject.toml #162
Conversation
nrontsis
changed the title
|
I'm willing to add this file if there aren't negative effects, but regardless can we leave the build deps in the docs. I'd like |
|
It is also worth noting that pyproject.toml does not handle build dependencies that can't be installed via pypi/pip, e.g. ipopt. |
|
@brocksam are you up on the info about this? I only have negative experiences with it, but I recognize it is likely our destiny. |
docs/source/install.rst
Outdated
| * setuptools | ||
| * cython | ||
| * numpy | ||
| * scipy [optional] |
There was a problem hiding this comment.
You've also removed scipy here as an optional dep. How does pyproject.toml handle that?
There was a problem hiding this comment.
My understanding is that the scipy interface of cyipopt is not related to the build process, so it has no place in build requiremetns.
There was a problem hiding this comment.
This list is a list of all build and run time dependencies (optional or not). I'd like to leave the list intact in the documentation.
There was a problem hiding this comment.
I have restored the diff here, but I find that docs confusing. They say:
To begin installing from source you will need to install the following dependencies
So I don't understand them to be "build or run time depedencies" necessarily.
Furthermore, no versions are specified here, which is inconsistent with what setup.py requires.
This is currently used in the CI setup and maybe even readthedocs. |
Yeah, I saw that. My opinion would be that it's best that the requirements of CI etc are better to be specified "closer" to the CI code, as the do for example here. Either way, it's beyond the scope of this PR. |
How about I instead write that "all the pypi requirements specified in |
|
There are relevant older conversations on this, e.g. #100 |
After reading this, I think we might have different workflows in mind. For example, I don't see the following as a reason to not try to eliminate custom python pre-build dependencies:
In workflows I work with, all python dependencies are resolved via a python package manager (like Having |
|
We should try to make this package work with new popular packaging tools, but we also need to make sure it continues to work with our currently supported packaging methods. This package predates poetry, wheels, and such. It is also traditionally has been a challenge to package due to a the dependencies (build and run) that are not available via pypi. A far as I understand, poetry still can't install this package fully because it can't install all of the necessary dependencies. If adding the pyproject file helps this work with poetry, we can include it, but we can't break the existing setup. |
See #41. If you can point us to a way to build wheels for the major operating systems in a way that works correctly with non-wheel ipopt installations, then we can adopt that here. I'm not aware of what that solution is. |
Do you have an example poetry configuration in mind? Here's what I would use that works with the patch of this PR:Dockerfile: FROM python:3.10-slim
WORKDIR /app
RUN apt-get update && \
apt-get install -y build-essential coinor-libipopt-dev liblapack-dev pkg-config
RUN pip install -U pip && pip install poetry
RUN poetry new .
RUN poetry add git+https://github.com/nrontsis/cyipopt.git@patch-2 # Using poetry add cyipopt fails currently
# Download example problem
RUN apt-get install -y curl
RUN curl -O https://raw.githubusercontent.com/nrontsis/cyipopt/master/examples/hs071.pyBuild and solve example problem: docker build . --tag cyipopt
docker run cyipopt poetry run python hs071.py
Sure. Do you have an minimal example setup that shows
I saw you opened #163 for this. I am not experienced on this, but I would look at the python interfaces of Also in my opinion having pre-built wheels for some architectures does not remove the need for this PR, as users might still want to compile themselves for some reason (for example running in a another architecture like Linux |
No, I don't personally use poetry for anything. This package has to work with a variety of downstream packaging or build tools, poetry is just one of them. Your example dockerfile that uses poetry works only on Debian based systems and require an apt-get installed ipopt. Yes, installing from source works cleanly (now) on Debian systems. This is explained in the install docs already, e.g. Has worked since Ubuntu 18.04 gained functional ipopt binaries. We also support Mac and Windows and other linux distros.
Can you report this error message here?
All these issues discuss various problems: conda-forge/conda-forge.github.io#1174 There are numerous conda packages that have to delete the pyproject.toml file before building. This one would likely have to do the same. So, if we package this
I've been reading these things all morning. It's a bit nasty to get working and will take some time, but at least I see that is it now possible and we can likely bundle ipopt in a wheel for all platforms.
I am open to adding this file specifically for poetry to function, but it is likely going to break the conda builds. I'm trying to understand if there are better ways to handle the incompatibilities. This project currently supports installing from conda, setup.py, and pip. We can add poetry support if we retain the currently supported methods. |
Here's what I got
The point I was trying to make is that assuming you have non-python system-level dependencies installed, poetry will work with the fix of this PR. The same point holds for other OSes, I believe; I can provide example scripts if you want.
The problem with the above approach is that a certain version of numpy (and the other python packages you install via apt-get) is used that might not be compatible with the requirements of a wider python project. In the example I have listed in my previous comment Similar complications exist for non-python system level dependencies, but I don't see these as a reason to not handle python dependency resolution as best as possible. Which in this case includes specifying as best as possible the python build dependencies.
As I described above, I don't think that
Thanks I will take a look at these. |
|
Here are key pieces of info from your error message: It looks like poetry invokes a very specific set of flags to pip. These are the flags: If poetry is disabling dependency installation, then it is expected to manually install the dependencies before invoking the install command. We also don't yet support PEP-517. One reason being that it breaks the conda builds. I haven't updated myself on the latest state of PEP-517 + conda builds, maybe there is now a compatible way to manage it. If there is, then we can adopt it for this package and add poetry support. I believe the package installs fine with pip if the no-deps and use-pep517 flags are not invoked, which is pip's long time default behavior. The deprecation warning is concerning though. It looks like the use of setup.py may be removed in the future. We'll be force to migrate at some point given that. The scenario for this package is:
So there is a bit of a catch 22 until we learn how all this works together. Some things that will help:
For now, we can drop this pyproject.toml in the repository, but there needs to be more investigation before we add it to the source distribution. |
You can install cyipopt alongside any other set of python dependencies as long as the numpy, cython, ipopt, and scipy version are above the minimally supported versions. This works in a virtual env or conda env, for example. Of course, the non-python deps need to be installed in some way other than with pip. So something like: Or if poetry has a flag to allow non-pep 517 compliant packages, that could be invoked. |
Adds
pyproject.tomlfile to specify the build dependencies for this package. This removes the need to have e.g.setuptoolsbefore runningpip install cyipopt.Docs are updated accordingly.
On a related note, I think it's worth considering removing
requirements.txtto reduce confusion.