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

Improve GitHub Action for testing #268

Closed
wants to merge 14 commits into from
Closed

Conversation

cthoyt
Copy link

@cthoyt cthoyt commented Feb 4, 2022

As I'm trying to figure out why #266 isn't working, I want to try adding a couple less controversial things to the existing testing workflow

  • Test on more modern versions of Python (3.6 has passed its end of life as of December 2021)
  • Test directly with pytest instead of setup.py (the Python Packge Authority has deprecated all usages of setup.py as a script
  • Add torch to install_requires in the setup.py to signify that this package requires torch after it's installed. You can always install a specific version of torch before trying to install torch-scatter and it will respect what's already available in the environment.

Things that didn't work:

  1. Testing on Mac OS~ (this doesn't work without getting rid of the +cpu)
  2. Removing the -e (editable mode) flag causes ImportError: Could not find module '_version_cpu' in /home/runner/work/pytorch_scatter/pytorch_scatter/torch_scatter
  3. Adding torch to setup_requires in the setup.py is a bit of a Catch-22: you already have to be executing the setup.py to recognize that. Getting this right can probably be done with the declarative setup that I proposed in Add build requirements in pyproject.toml to make installations easier #266, but that one still needs a few things to be clarified (plus CI to pass)

@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2022

Codecov Report

Merging #268 (15af45a) into master (fc1b139) will decrease coverage by 0.49%.
The diff coverage is n/a.

❗ Current head 15af45a differs from pull request most recent head 18679be. Consider uploading reports for the commit 18679be to get more accurate results

@@            Coverage Diff             @@
##           master     #268      +/-   ##
==========================================
- Coverage   97.54%   97.05%   -0.50%     
==========================================
  Files           9        9              
  Lines         204      204              
==========================================
- Hits          199      198       -1     
- Misses          5        6       +1     
Impacted Files Coverage Δ
torch_scatter/scatter.py 96.00% <0.00%> (-2.00%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@cthoyt cthoyt marked this pull request as ready for review February 4, 2022 00:44
@cthoyt
Copy link
Author

cthoyt commented Feb 4, 2022

@rusty1s this is a bit more baked than #266 and has a few notes on what works and what doesn't - it's ready for review if you've got some time :)

setup.py Outdated
setup_requires = []
tests_require = ['pytest', 'pytest-runner', 'pytest-cov']
tests_require = ['pytest', 'pytest-cov', 'coverage']
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding coverage here is unnecessary as this is only used in CI, not on a local dev machine. Happy to remove pytest-runner though :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, fair point!

@@ -84,9 +84,9 @@ def get_extensions():
return extensions


install_requires = []
install_requires = ['torch']
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still have my problems with this, but mostly only due to bad experience in the past. For example, it is impossible for me to install the sentence-transformers library since due to whatever reasons PyTorch will always get re-installed.

It also looks like pip install torch-scatter will still fail when no PyTorch version is installed, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will see if I can replicate this problem using some clean virtual environments

@github-actions
Copy link

github-actions bot commented Aug 4, 2022

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 Aug 4, 2022
@github-actions github-actions bot removed the stale label Aug 5, 2022
@github-actions
Copy link

github-actions bot commented Feb 2, 2023

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 Feb 2, 2023
@rusty1s rusty1s added enhancement and removed stale labels Feb 2, 2023
@github-actions
Copy link

github-actions bot commented Aug 2, 2023

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 Aug 2, 2023
@github-actions github-actions bot closed this Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants