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

fix(setup): add torch as build system requirement #1479

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wsascha
Copy link

@wsascha wsascha commented Mar 16, 2023

What?

I've added a new pyproject.toml file to the project that contains static information according to PEP 517.

I've also added torch as build system requirement so that isolated installation via setuptools (and also poetry and probably most other PEP 517 compliant build frontends) works.

Tests?

I've tested this via

pip install --isolated --verbose --editable .

from within the project directory (and via poetry when I provide this project as path dependency).

Anything else?

I should also say that I have no clue about any other specifics that might make the installation or packaging fail and just did this to solve my current problem of integrating pytorch3d into poetry. Anyway, I'm also interested in getting your feedback on this..

WDYT?

...also move static information from setup.py to pyproject.toml
@facebook-github-bot
Copy link
Contributor

Hi @wsascha!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@wsascha wsascha changed the title fix: add torch as build system requirement fix(setup): add torch as build system requirement Mar 16, 2023
@bottler
Copy link
Contributor

bottler commented Mar 16, 2023

We were discussing this on #1419 . I don't think this is the right kind of change. But also I don't really understand the problem you are trying to solve. From my point of view, I generally want all users to pick their version of PyTorch manually.

@wsascha
Copy link
Author

wsascha commented Mar 20, 2023

There's a difference between "I want users to pick their torch version manually" and "I need to have torch installed before I can even know what dependencies this library has". As far as I can see, your library's deps (and a lot of other project meta data) are not even dependent on a specific torch version. – They are just passed to the setup function call. So, I'm wondering why you don't define this project information in a separate file that can be read by modern tools (e.g. to build a conflict-free dependency tree) before running that setup.py to get the remaining information depending on the torch version.

Anyway, I figured there's currently a blocker for adding torch as build requirement as pip exactly does what you are afraid of – just installing the latest version of build requirements instead of choosing the already installed one: pypa/pip#9542

I consider this a bug which is resolved sooner or later. If you'd still like me to make other project information available in a PEP 517 compliant way, I can adapt this pr. – let me know what you think.

@bottler
Copy link
Contributor

bottler commented Mar 21, 2023

When you say you "consider this a bug", do you mean the behaviour of pip or this issue in pytorch3d? Unless someone can clearly explain how not to break things which are currently working, pytorch3d isn't going to change on this soon and there isn't a bug in pytorch3d. The discussion might as well be had on #1419 though, not here.

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

Successfully merging this pull request may close these issues.

3 participants