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.

Sign up for GitHub

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

Jump to bottom

Add pyproject.toml with PEP 517 metadata #291

Closed
wants to merge 1 commit into from
Closed

Add pyproject.toml with PEP 517 metadata #291

wants to merge 1 commit into from

Conversation

JacobHayes
Copy link

@JacobHayes JacobHayes commented Nov 17, 2022 •
edited
Loading

This adds a pyproject.toml with a build-system.requires section that includes torch, which triggers torch to be installed before building torch_sparse. One caveat might be what you mention in #157 (comment) that the specific version of torch (both direct version + CUDA version) would be unspecified, but my understanding is it would basically pick the latest version available (I think this might still be more desirable than the current errors?). The default torch wheels when building on linux are compiled with CUDA support (I think the default 1.13 wheels uses CUDA 11.7).

If acceptable, this should fix #156.

@codecov-commenter
Copy link

Codecov Report

Merging #291 (4d290af) into master (d1aee18) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #291   +/-   ##
=======================================
  Coverage   71.29%   71.29%           
=======================================
  Files          25       25           
  Lines        1066     1066           
=======================================
  Hits          760      760           
  Misses        306      306

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@JacobHayes
Copy link
Author

Well, I guess the test failures aren't a great sign. 😁 I'm not exactly sure how only the windows-2019, 3.7, 1.13.0 one passed, but all the others have different failures.

I think adding the pyproject.toml triggers an isolated build of the package, so it probably compiled with different versions than are pinned in CI and installed otherwise?

@JacobHayes JacobHayes marked this pull request as draft November 17, 2022 04:13
@rusty1s
Copy link
Owner

rusty1s commented Nov 17, 2022

Thanks for the PR. I personally think this is very dangerous to do. Most of my reasons have been described here: rusty1s/pytorch_scatter#266

The main problem is that torch dependency might come with a CUDA version not available on your system, which lets installation fail.

@JacobHayes
Copy link
Author

Drat, I searched in only one of the repositories. 😁 Thanks for the link - I'll go ahead and close!

@JacobHayes JacobHayes closed this Nov 17, 2022
@JacobHayes JacobHayes mentioned this pull request Nov 17, 2022
Open
@rusty1s
Copy link
Owner

rusty1s commented Nov 17, 2022

Yeah, I don't think there is an elegant solution for this on pip side. conda should work just fine though if you are interested in intstalling PyTorch dependency as well as it can sync the CUDA dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error in setup.py "No module named 'torch'" when installing with Poetry
3 participants
@JacobHayes @codecov-commenter @rusty1s