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

Wheel build - Skip tests for cp37 and cp38-macosx_x86_64 #3015

Merged
merged 2 commits into from Jun 22, 2023

Conversation

PrimozGodec
Copy link
Contributor

@PrimozGodec PrimozGodec commented Jun 21, 2023

Overview

Description of the changes proposed in this pull request:
Fix tests in the wheel build

  • Skip cp37: since many packages that Shap depend on do not provide cp37 wheel anymore. They would need to be built before tests can run. One of the packages is Catboost which would need Ninja to build; also, if Ninja is installed, it takes a long time to build, so I suggest skipping cp37.
  • Skip cp38-macosx_x86_64: In the MacOS and Python 3.8 case, pip will not download the universal Catboost wheel but will take .tar.gz and try to build it. It is likely due to: GitHub actions Build fails on macOS with python 3.8 - catboost 1.2 catboost/catboost#2371

Checklist

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #3015 (32a60a8) into master (edb3464) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3015      +/-   ##
==========================================
- Coverage   54.79%   54.78%   -0.01%     
==========================================
  Files          90       90              
  Lines       12895    12895              
==========================================
- Hits         7066     7065       -1     
- Misses       5829     5830       +1     

see 1 file with indirect coverage changes

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

Copy link
Collaborator

@connortann connortann left a comment

Choose a reason for hiding this comment

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

This looks good, thanks for the PR!

Just one request, please add a comment in the YAML file to explain the rationale behind the skipped tests, that the catboost wheel is not available. You could reference this PR number for more info.

FYI I edited the description to change "closes #3012" to "related: #3012", as I don't think this PR by itself will fully close that issue.

pyproject.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@connortann connortann left a comment

Choose a reason for hiding this comment

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

@PrimozGodec I noticed one other change to address, please see the thread above.

@connortann connortann added this to the 2023-06-fixes milestone Jun 21, 2023
@connortann connortann added the ci Relating to Continuous Integration / GitHub Actions label Jun 21, 2023
@PrimozGodec PrimozGodec marked this pull request as draft June 22, 2023 06:34
@PrimozGodec
Copy link
Contributor Author

I changed the PR to draft since I want to test what work and what doesn't will change it to ready later

@connortann
Copy link
Collaborator

I would like to get the main changed merged in soon, so we can keep progressing towards a release. So, if the win32 build issue is not something that can be resolved quickly, I'd suggest removing that from the scope of this PR and raising a separate issue.

@@ -12,6 +12,7 @@ jobs:
name: Build wheels on ${{ matrix.os }}
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this for easier debugging. That one build does not finish because of another. E.g. the Windows build does not finish because of failing MacOS. The whole building will still fail at the end, and no upload will happen.

@PrimozGodec PrimozGodec marked this pull request as ready for review June 22, 2023 09:47
@PrimozGodec
Copy link
Contributor Author

I would like to get the main changed merged in soon, so we can keep progressing towards a release. So, if the win32 build issue is not something that can be resolved quickly, I'd suggest removing that from the scope of this PR and raising a separate issue.

Good. I made a PR ready to merge and will continue with debugging in a separate PR after #3021 and #3020 are merged.

Copy link
Collaborator

@connortann connortann left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM! I merged in master to update the branch, so will merge once the tests pass.

@connortann connortann merged commit de10c51 into shap:master Jun 22, 2023
8 of 9 checks passed
@PrimozGodec PrimozGodec deleted the fix-wheel-tests branch June 22, 2023 10:22
@connortann connortann modified the milestones: 2023-06-fixes, 0.42.0 Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Relating to Continuous Integration / GitHub Actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants