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

Allow poetry add to force build dependencies from source #5599

Closed
2 tasks done
ajatkj opened this issue May 12, 2022 · 12 comments · Fixed by #5600
Closed
2 tasks done

Allow poetry add to force build dependencies from source #5599

ajatkj opened this issue May 12, 2022 · 12 comments · Fixed by #5600
Labels
kind/feature Feature requests/implementations

Comments

@ajatkj
Copy link

ajatkj commented May 12, 2022

  • I have searched the issues of this repo and believe that this is not a duplicate.
  • I have searched the documentation and believe that my question is not covered.

Feature Request

Can we have an option in poetry add to build a dependency from source instead of just downloading the existing package for the platform?
I am talking something equivalent to pip3 install --no-binary :all: <package> --ignore-installed.

Current work-around is to use poetry run pip3 install .... and then do poetry add --lock to update the lock file. It will be good to have a flag in add to do this easily.

@ajatkj ajatkj added kind/feature Feature requests/implementations status/triage This issue needs to be triaged labels May 12, 2022
@abn
Copy link
Member

abn commented May 13, 2022

--no-binary for add, update or lock does not make sense. The only case where i makes sense is for the install command.

If you need to avoid installation during add you can do the following (with #5600).

poetry add --lock package
poetry install --no-binary package

@clintonroy
Copy link
Contributor

If it's not remembered in pyproject.toml, every person building the project will have to be told to do the --no-binary install option though, no?

@abn
Copy link
Member

abn commented May 13, 2022

Perhaps. However, this is a user preference and not a project preference I believe. I'm unsure if a project level preference for this really makes sense. That would mean we lock only source distribution etc (which is weird when a bdist is available) and further there is no way to put this information in the project's metadata when building bdist or sdists.

@ajatkj
Copy link
Author

ajatkj commented May 13, 2022

@abn what you say does make sense as this is not exactly a project level preference. The work around to use poetry install --no-binary after poetry add --lock <package> does the work for me. I will be closing this ticket now. Thanks for everyone's comments.

@ajatkj ajatkj closed this as completed May 13, 2022
@pmav99
Copy link
Contributor

pmav99 commented May 14, 2022

@abn I am afraid that this can be a project preference and/or even necessity. A common issue with geospatial packages is that they bundle e.g. GEOS inside the wheel. When you need more than one of these packages and the GEOS binaries are different you run into trouble. E.g.: geopandas/geopandas#2355

To replicate with poetry:

poetry init -n
poetry add shapely pygeos geopandas
python -c 'import geopandas'

will result in performance issues (and/or strange bugs)

/tmp/asdf/.venv/lib/python3.9/site-packages/geopandas/_compat.py:111: UserWarning: The Shapely GEOS version (3.10.2-CAPI-1.16.0) is incompatible with the GEOS version PyGEOS was compiled with (3.10.1-CAPI-1.16.0). Conversions between both will be slow.

To circumvent this you need to pretty much always use the source distribution for both shapely and pygeos.

So, if I understand this correctly, in order to install the source distributions you have to do it like this:

poetry init -n
poetry add --lock shapely pygeos geopandas
poetry install --no-binary=shapely,pygeos
python -c 'import geopandas'

which indeed works. The question is what happens when a new shapely or pygeos version gets released? I assume that the next time you run poetry update you will get the binary version. Therefore updates will also need to be done with:

poetry update --lock
poetry install --no-binary=shapely,pygeos

Am I wrong?

@abn
Copy link
Member

abn commented May 14, 2022

You are not wrong in the update case. You can also use poetry lock.

The implementation I have proposed is purely focussed on cases where a project is being deployed. I might revert the change and reconsider a broader implementation. However, this will introduce a range of other issues.

  1. What happens when a project is packaged? Do we mark it as a direct reference to sdist?
  2. How do we mitigate performance issues incurred if we discard binaries when generating metadata?

I still do think the project should enforce what distribution is used by the user when being installed. Maybe it is better off as a poetry config value? That only applies if your case is not the 80% case.

@abn
Copy link
Member

abn commented May 14, 2022

@pmav99 while still not project specific, I suspect #5609 should help in your case. As an interim measure you could commit your poetry.toml along with your project to allow for your users to make use of this.

poetry config --local installer.no-binary shapely,pygeos

@pmav99
Copy link
Contributor

pmav99 commented May 14, 2022

@abn thank you very much for taking the time to further look into this. I understand that these geospatial packages are kind of a niche field, but they are an existing pain point WRT to poetry usage/adoption.

As far as the metadata goes, I am afraid I don't have much to offer. I have never had a close look at the relevant PEPs. Nevertheless, it feels that forcing binary or source distributions at the package level is somewhat questionable. The choice of which distribution to install should be made downstream by whomever deploys the software. Therefore a package should not contain such information. For example, using the binary distribution might be OK in certain cases. E.g. you might not have a compiler where you deploy, or you might be missing the GEOS library or whatever. In these cases you want the binary package, even if it means degraded performance.

That being said, i don't consider the deployment of a package a huge issue. Pip can handle it just fine. E.g.

pip install ./ --no-binary shapely --no-binary pygeos
# or
pip install -r requirements.txt --no-binary shapely --no-binary pygeos

The question is what do you do while developing. And this brings me to the thing that I am not really fond of with #5600, i.e. the fact that the commands to install/upgrade stop being simple to use. Practically speaking, you will never be able to use plain poetry add and poetry update while developing. You will always have to use poetry update --lock && poetry install --no-binary=LIST_OF_PACKAGES. OK, you can automate/simplify this by using e.g. a Makefile, but 1. whenever you add/remove a non-binary package you will need to edit both pyproject.toml and Makefile in order to keep them in sync. 2. Passing arguments to Makefile for poetry add is painful. Not the end of the world, but still...

At first glance, from a UX point of view, #5609 seems a much better workaround to me. That being said, it could be argued that package specific configurations don't really belong in poetry.toml. E.g. why not add this information in pyproject.toml, instead? poetry could ignore it when bundling a package but could use it when installing packages. Anyhow, I will test it out and report on the PR.

@abn
Copy link
Member

abn commented May 14, 2022

The thing is, it's not package specific but rather environment specific. As in, chances are if you want sdist for a particular package, you'd need that for other projects on the system as well. Applies the other way around too.

And poetry right now is strict when parsing pyproject toml. This means poetry-core will need to allow this first etc. And I don't think there is a good place in there for this either. I'd be against putting this in the dependency specification because as I said this is an environment configuration not a project configuration per say. Atleast for now I do not see this changing.

@mkniewallner mkniewallner removed the status/triage This issue needs to be triaged label Jun 11, 2022
@zacnam
Copy link

zacnam commented Jul 20, 2022

poetry config --local installer.no-binary

@abn Hi abn, trying to implement this command with grpcio as it can't build on Mac M1 with the binary, but am getting the error Setting installer.no-binary does not exist.
Have tried adding it manually to my config.toml under the Library/Application Support, but then it throws the error 'no-binary' was unexpected - can you please help me understand how this flag can be used?

@ajatkj
Copy link
Author

ajatkj commented Jul 25, 2022

```shell
poetry config --local installer.no-binary

@abn Hi abn, trying to implement this command with grpcio as it can't build on Mac M1 with the binary, but am getting the error Setting installer.no-binary does not exist.
Have tried adding it manually to my config.toml under the Library/Application Support, but then it throws the error 'no-binary' was unexpected - can you please help me understand how this flag can be used?

This is added in version 1.2 here which is still in beta. You can install beta to use this feature

Copy link

github-actions bot commented Mar 1, 2024

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/feature Feature requests/implementations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants