-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: remove extra arch + refactor unit tests workflow #2
Conversation
if: ${{matrix.os == 'macos-latest'}} | ||
run: pip install torch==${{ matrix.torch-version }} | ||
|
||
pip install "numpy==1.26.4" scikit-learn flake8 setuptools numba |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious, why are the other dependencies not pinned to a specific version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've received an error from Torch - Numpy interaction and this is the version we have in OSS dependencies currently.
The public repository doesn't have any changes to the unit tests workflow for 2 years - they don't support Python 3.8 yet so I thought this is already much better than what there was 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did this work with the matrix, did we test for all os x python version x torch versions? And now we only will test with ubuntu latest and torch?
|
||
jobs: | ||
unittests: | ||
strategy: | ||
matrix: | ||
os: [ubuntu-latest, macos-latest, windows-latest] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was not listed as a goal of this PR. Why don't we need macos and windows anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't run anything on macOS or Windows so I didn't want us to waste time / resources with those.
CUDA 12 is not compatible with some of the older GPU architectures, namely:
torch-points-kernels setup file explicitly adds support for the architecture
sm_35
for a currently unknown reason (see more context).This PR removes this additional build flag.
Also, I refactored unit tests workflow to adjust some stuff to our current setup.
The plan is to issue a new release,
0.7.2
after merging this.