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

Consolidate C++ conda recipes and add libraft-tests package #641

Merged
merged 5 commits into from
Jun 3, 2022

Conversation

jjacobelli
Copy link
Contributor

@jjacobelli jjacobelli commented May 6, 2022

This PR includes the following changes:

  • Adds a libraft-tests package to the libraft-split recipe
    • This is a prerequisite for removing "Project Flash" from our build/CI scripts
    • The libraft-tests package was added as an additional output to the libraft-split recipe
  • Consolidates remaining C++ recipes into libraft-split recipe
    • This gets rid of a lot of duplicate code between the recipes and reduces the number of times we have to call conda build in our CI scripts
  • Migrate the "from sources" builds done in GPU tests by building packages using conda. This is done for the following reasons:
    • This is required step to improve the Ops CI/CD setup to a more convenient pipeline
    • This is required to start using conda compilers and mamba to build RAPIDS packages
    • This prevent us from manually managing and installing the dependencies in GPU job
    • This ensure the packages can be installed (no conda conflict while installing)
    • This ensure the tests are running and working against the package content and not the build results. Currently the Python packages are not tested

Dependency version specs are now specified in conda/recipes/libraft/conda_build_config.yaml. The exception here is the version spec used for cudatoolkit since that comes from an environment variable in the CI process.

@jjacobelli jjacobelli added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 6, 2022
@jjacobelli jjacobelli self-assigned this May 6, 2022
@github-actions github-actions bot added the gpuCI label May 6, 2022
@jjacobelli jjacobelli force-pushed the consolidate branch 5 times, most recently from 5f6f2f2 to e622f73 Compare May 6, 2022 12:40
@github-actions github-actions bot added the python label May 6, 2022
@jjacobelli
Copy link
Contributor Author

rerun tests

@jjacobelli jjacobelli force-pushed the consolidate branch 2 times, most recently from 92e8ca2 to 8ad57f5 Compare May 9, 2022 11:46
@jjacobelli jjacobelli marked this pull request as ready for review May 9, 2022 12:48
@jjacobelli jjacobelli requested review from a team as code owners May 9, 2022 12:48
@cjnolet
Copy link
Member

cjnolet commented May 10, 2022

@Ethyling This would be a good PR to activate the staging conda label. Any chance we could do that here? This would allow us to physically compare the deployed artifacts before consuming projects use them.

@jjacobelli
Copy link
Contributor Author

jjacobelli commented May 10, 2022

@Ethyling This would be a good PR to activate the staging conda label. Any chance we could do that here? This would allow us to physically compare the deployed artifacts before consuming projects use them.

Yes, let me update the PR with this
EDIT: PR updated with this

@jjacobelli jjacobelli force-pushed the consolidate branch 2 times, most recently from 0bc5365 to 26f4925 Compare May 10, 2022 15:57
@cjnolet
Copy link
Member

cjnolet commented May 20, 2022

@Ethyling I had to back out a couple recently merged PRs at the beginning of the week because they had caused some builds to break downstream (cuml specifically).

IIRC, the changes in those PRs were intended to fix the cmake exports from consolidating and the build steps in this PR, right?

@jjacobelli
Copy link
Contributor Author

@Ethyling I had to back out a couple recently merged PRs at the beginning of the week because they had caused some builds to break downstream (cuml specifically).

IIRC, the changes in those PRs were intended to fix the cmake exports from consolidating and the build steps in this PR, right?

@cjnolet Yes. This PR requires to have a CMake component installation working. Maybe we can try to work on fixing the issue you were facing with cuml?

@jjacobelli jjacobelli force-pushed the consolidate branch 2 times, most recently from 8224d06 to ef6f83a Compare June 1, 2022 08:51
@cjnolet
Copy link
Member

cjnolet commented Jun 1, 2022

@vyasr CI in this PR seems to be indicating that the cmake for pylibraft cannot find the distance library even though it's being built and installed earlier in the build script. Have you seen this behavior or know how to handle it?

@jjacobelli jjacobelli force-pushed the consolidate branch 2 times, most recently from 425b4bb to b63c3cb Compare June 1, 2022 19:27
@vyasr
Copy link
Contributor

vyasr commented Jun 1, 2022

No, I haven't seen this issue before. Right now what I see in CI is that it's not finding cuco, and you're saying that implicates the distance lib because cuco should be exported as a global target from there, right?

@cjnolet
Copy link
Member

cjnolet commented Jun 1, 2022

@vyasr talking with @robertmaynard and @Ethyling offline, we came to the realization that the changes in this PR to consolidate the 3 c++ components into a single build is not going to work so we are going to keep that part the way it is for now.

To follow up about my earlier question- the problem is that when we run cmake install on an individual component, it's only going to install that component, so dependencies will not be installed.

@jjacobelli jjacobelli requested a review from a team as a code owner June 3, 2022 07:29
@jjacobelli jjacobelli changed the base branch from branch-22.06 to branch-22.08 June 3, 2022 07:29
Signed-off-by: Jordan Jacobelli <[email protected]>
Signed-off-by: Jordan Jacobelli <[email protected]>
Signed-off-by: Jordan Jacobelli <[email protected]>
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

The resulting packages seem to look correct at a glance. Thanks again @Ethyling!

conda/recipes/libraft/build.sh Outdated Show resolved Hide resolved
@jjacobelli
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 3c3cefd into rapidsai:branch-22.08 Jun 3, 2022
@ajschmidt8 ajschmidt8 mentioned this pull request Jun 3, 2022
cjnolet pushed a commit to cjnolet/raft that referenced this pull request Jun 6, 2022
…#641)

This PR includes the following changes:
- Adds a `libraft-tests` package to the `libraft-split` recipe
  - This is a prerequisite for removing "Project Flash" from our build/CI scripts
   - The `libraft-tests` package was added as an additional output to the `libraft-split` recipe
- Consolidates remaining C++ recipes into `libraft-split` recipe
   - This gets rid of a lot of duplicate code between the recipes and reduces the number of times we have to call conda build in our CI scripts
- Migrate the "from sources" builds done in GPU tests by building packages using conda. This is done for the following reasons:
  - This is required step to improve the Ops CI/CD setup to a more convenient pipeline
  - This is required to start using conda compilers and `mamba` to build RAPIDS packages
  - This prevent us from manually managing and installing the dependencies in GPU job
  - This ensure the packages can be installed (no conda conflict while installing)
  - This ensure the tests are running and working against the package content and not the build results. Currently the Python packages are not tested

Dependency version specs are now specified in `conda/recipes/libraft/conda_build_config.yaml`. The exception here is the version spec used for cudatoolkit since that comes from an environment variable in the CI process.

Authors:
  - Jordan Jacobelli (https://github.com/Ethyling)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)
  - AJ Schmidt (https://github.com/ajschmidt8)

URL: rapidsai#641
ajschmidt8 added a commit that referenced this pull request Jun 7, 2022
Our `devel` Docker containers need to be switched to using `conda` compilers to resolve a linking error. `raft` is in those containers, but hasn't yet been built with `conda` compilers. This PR addresses that.

These changes won't cleanly merge into `branch-22.08` unfortunately due to the changes in #641, but we can address that another time.

Authors:
   - AJ Schmidt (https://github.com/ajschmidt8)
   - Corey J. Nolet (https://github.com/cjnolet)
   - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
   - Corey J. Nolet (https://github.com/cjnolet)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake cpp gpuCI improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants