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 build requirements in pyproject.toml to make installations easier #266

Closed
wants to merge 9 commits into from
Closed

Add build requirements in pyproject.toml to make installations easier #266

wants to merge 9 commits into from

Conversation

cthoyt
Copy link

@cthoyt cthoyt commented Feb 2, 2022 •
edited
Loading

Closes #265

This pull request does the following:

  1. Adds torch as a build requirement using pyproject.toml as described in https://setuptools.readthedocs.io/en/latest/build_meta.html. The use of setup.py is getting towards the end of life, and using a combination of declarative setup metadata in setup.cfg and pyproject.toml is the preferred way forwards. This means that if torch isn't already installed before doing pip install torch-scatter, it gets installed.
  2. Adds torch as a install requirement (i.e., a dependency). This might be a bit redundant of 1, but some tools like tox create a separate env for the build requirements, so this needs to be duplicated.
  3. Adds an alternative testing scenario in which PyTorch is not installed before trying to pip install torch-scatter. This demonstrates that with the first two changes, it's now possible to make an automated, single step installation of torch-scatter.

Why is this important

Creating easily installable packages that depend on torch-scatter (that don't include the need for manual installation of torch + cuda) is not currently possible because pip's dependency resolver isn't aware of the fact that torch-scatter needs torch installed before even trying to build it.

@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2022 •
edited
Loading

Codecov Report

Merging #266 (d46aba3) into master (2f447cf) will not change coverage.
The diff coverage is n/a.

❗ Current head d46aba3 differs from pull request most recent head 2a3f89e. Consider uploading reports for the commit 2a3f89e to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #266   +/-   ##
=======================================
  Coverage   97.05%   97.05%           
=======================================
  Files           9        9           
  Lines         204      204           
=======================================
  Hits          198      198           
  Misses          6        6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f447cf...2a3f89e. Read the comment docs.

@cthoyt cthoyt mentioned this pull request Feb 2, 2022
Closed
1 task
@rusty1s
Copy link
Owner

rusty1s commented Feb 3, 2022

Any idea why tests are failing?

@cthoyt cthoyt mentioned this pull request Feb 4, 2022
Closed
3 tasks
@gchaperon
Copy link

any updates on this feature?
it would be great to just be able to do pip install torch-scatter, this would help a lot to automate environmet setup :)

@rusty1s
Copy link
Owner

rusty1s commented Apr 16, 2022

This is likely not going to happen since pip install torch-scatter will pick up the local CUDA environment, not the default one provided by pip install torch. This will result in a lot of problems later on. We also want to discourage manual compilation and recommend installation from pre-built wheels.

@MichelBartels MichelBartels mentioned this pull request May 3, 2022
Closed
4 tasks
@github-actions
Copy link

This pull request had no activity for 6 months. It will be closed in 2 weeks unless there is some new activity.

@github-actions github-actions bot added the stale label Oct 14, 2022
@github-actions github-actions bot closed this Oct 29, 2022
@rusty1s rusty1s mentioned this pull request Nov 17, 2022
Closed
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
stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Issues from importing torch in setup.py
4 participants
@cthoyt @codecov-commenter @rusty1s @gchaperon