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 CI scripts to build conda packages and run tests #18

Merged
merged 27 commits into from
Apr 11, 2023

Conversation

pentschev
Copy link
Member

No description provided.

@pentschev pentschev force-pushed the ci-support branch 2 times, most recently from 6922ddd to bda7f2a Compare April 7, 2023 11:05
@ajschmidt8 ajschmidt8 mentioned this pull request Apr 10, 2023
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Peter! 🙏

Had one question about RAPIDS release version bumps are handled (with respect to rmm version used)

Comment on lines 13 to 14
rmm_version:
- "23.04.*"
Copy link
Member

Choose a reason for hiding this comment

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

Does this get updated by the ci/release/update-version.sh script? If not, should it be?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks John, done in 3707568 .

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Comments attached.

branches:
- "branch-*"
tags:
- v[0-9].[0-9][0-9].[0-9][0-9]
Copy link
Contributor

Choose a reason for hiding this comment

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

I am guessing the major component is fixed as one digit because this aims to follow ucx versioning? Like 0.30.0? But then is the patch version also one digit or is it two digits? Like 0.30.00? Not sure what regex syntax is allowed here but I think there are limitations.

Copy link
Member Author

Choose a reason for hiding this comment

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

We're doing here exactly what UCX-Py is doing, I believe this is right as it has been for a while.

Comment on lines 108 to 116
specific:
- output_types: conda
matrices:
- matrix:
cuda: "11.8"
packages:
- cuda-sanitizer-api=11.8.86
- matrix:
packages:
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is cuda-sanitizer-api being used? I searched briefly but didn't see it. If it's not used, the entire specific section can be deleted.

Suggested change
specific:
- output_types: conda
matrices:
- matrix:
cuda: "11.8"
packages:
- cuda-sanitizer-api=11.8.86
- matrix:
packages:

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, removed.

- checks
- cudatoolkit
- py_version
- run
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- run
- run_python

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

- matrix:
packages:
- python>=3.8,<3.11
run:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
run:
run_python:

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

- &gmock gmock==1.10.0.*
- spdlog>=1.11.0,<1.12
- cython>=0.29,<0.30
- numpy>=1.21
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use an anchor here and re-use this pinning version in the other places numpy is referenced? See &gtest / *gtest for an example.

- python>=3.8,<3.11
run:
common:
- output_types: [conda, requirements]
Copy link
Contributor

Choose a reason for hiding this comment

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

Some additional work is probably needed to make this file compatible with generating the build/run/test requirements in pyproject.toml. Happy to defer that to a later PR if we can file an issue.

Suggested change
- output_types: [conda, requirements]
- output_types: [conda, requirements, pyproject]

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

This file also needs to update rmm_version in conda/recipes/ucxx/conda_build_config.yaml.

conda activate checks

# Run pre-commit checks
pre-commit run --hook-stage manual --all-files --show-diff-on-failure
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pre-commit run --hook-stage manual --all-files --show-diff-on-failure
pre-commit run --all-files --show-diff-on-failure

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend updating the hook versions in this file with pre-commit autoupdate. Since it's a new package, that should be a better place to start. I am going to be updating hook versions in other RAPIDS repos in the next month or so.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't mind if we do it in a follow-up PR, but I'd rather have something functional first and iterate on improvements later.

jobs:
conda-cpp-tests:
secrets: inherit
uses: rapidsai/shared-action-workflows/.github/workflows/[email protected]
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems wrong.

Suggested change
uses: rapidsai/shared-action-workflows/.github/workflows/conda-python-[email protected]
uses: rapidsai/shared-action-workflows/.github/workflows/conda-cpp-tests[email protected]

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member Author

@pentschev pentschev left a comment

Choose a reason for hiding this comment

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

Thanks @jakirkham and @bdice , I've addressed the suggestions

branches:
- "branch-*"
tags:
- v[0-9].[0-9][0-9].[0-9][0-9]
Copy link
Member Author

Choose a reason for hiding this comment

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

We're doing here exactly what UCX-Py is doing, I believe this is right as it has been for a while.

jobs:
conda-cpp-tests:
secrets: inherit
uses: rapidsai/shared-action-workflows/.github/workflows/[email protected]
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't mind if we do it in a follow-up PR, but I'd rather have something functional first and iterate on improvements later.

Comment on lines 13 to 14
rmm_version:
- "23.04.*"
Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks John, done in 3707568 .

Comment on lines 108 to 116
specific:
- output_types: conda
matrices:
- matrix:
cuda: "11.8"
packages:
- cuda-sanitizer-api=11.8.86
- matrix:
packages:
Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, removed.

- checks
- cudatoolkit
- py_version
- run
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

- matrix:
packages:
- python>=3.8,<3.11
run:
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

- python>=3.8,<3.11
run:
common:
- output_types: [conda, requirements]
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

conda activate checks

# Run pre-commit checks
pre-commit run --hook-stage manual --all-files --show-diff-on-failure
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@pentschev pentschev merged commit 58b1d47 into rapidsai:main Apr 11, 2023
@pentschev pentschev deleted the ci-support branch April 11, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants