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

Add ninja as build dependency #301

Merged
merged 9 commits into from
Nov 15, 2022
Merged

Add ninja as build dependency #301

merged 9 commits into from
Nov 15, 2022

Conversation

ajschmidt8
Copy link
Member

@ajschmidt8 ajschmidt8 commented Nov 9, 2022

Description

This PR adds ninja and make as build dependencies of rapids-cmake. Additionally, it disables the sccache S3 backend for the c++ tests since compile times are negligible.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • The cmake-format.json is up to date with these changes.
  • I have added new files under rapids-cmake/
    • I have added include guards (include_guard(GLOBAL))
    • I have added the associated docs/ rst file and update the api.rst

This PR adds `ninja` as a build dependency of `rapids-cmake`.
@ajschmidt8 ajschmidt8 added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Nov 9, 2022
@ajschmidt8 ajschmidt8 requested a review from a team as a code owner November 9, 2022 20:19
@jakirkham
Copy link
Member

Did we want to add make here too (based on offline discussion)?

@robertmaynard
Copy link
Contributor

Did we want to add make here too (based on offline discussion)?

Good call, yes we do.

dependencies.yaml Show resolved Hide resolved
@ajschmidt8
Copy link
Member Author

ajschmidt8 commented Nov 9, 2022

hmmmm. something about the changes in this PR is making the test script take exponentially longer than it has in the past.

here's an old run for comparison:

@ajschmidt8
Copy link
Member Author

I canceled the workflow for now to prevent wasting GPU resources since they're limited in GH Actions. we'll have to investigate why these changes are making things slower.

@jakirkham
Copy link
Member

Think make needs to be only Conda

dependencies.yaml Show resolved Hide resolved
dependencies.yaml Outdated Show resolved Hide resolved
@ajschmidt8
Copy link
Member Author

ajschmidt8 commented Nov 15, 2022

hmmmm. something about the changes in this PR is making the test script take exponentially longer than it has in the past.

here's an old run for comparison:

As it turns out, the slowdown was caused by the CI image changes below. Adding ENV SCCACHE_BUCKET=rapids-sccache to the CI images enables the S3 backend for sccache, but the proper AWS credentials weren't available to this repository. I've added those credentials to the rapids-cmake repository and it seems that that has resolved the slowdown issue.

EDIT ---

I've also added the changes in 7fac4f5 to disable the sccache S3 backend since compile times for these files are negligible.

@ajschmidt8
Copy link
Member Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 420e27b into branch-22.12 Nov 15, 2022
@ajschmidt8 ajschmidt8 deleted the add-ninja branch November 15, 2022 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants