Skip to content

[BUG] NEIGHBORS_ALL_NEIGHBORS_TEST build ignores --no-mg#1230

Merged
rapids-bot[bot] merged 12 commits intorapidsai:branch-25.10from
enp1s0:fix-NEIGHBORS_ALL_NEIGHBORS_TEST-build-with-no-mg
Aug 28, 2025
Merged

[BUG] NEIGHBORS_ALL_NEIGHBORS_TEST build ignores --no-mg#1230
rapids-bot[bot] merged 12 commits intorapidsai:branch-25.10from
enp1s0:fix-NEIGHBORS_ALL_NEIGHBORS_TEST-build-with-no-mg

Conversation

@enp1s0
Copy link
Copy Markdown
Member

@enp1s0 enp1s0 commented Aug 8, 2025

The following error occurs when building the unit tests on a machine where NCCL is not installed, even though the --no-mg option is specified.

CMake Error at tests/CMakeLists.txt:47 (target_link_libraries):
  Target "NEIGHBORS_ALL_NEIGHBORS_TEST" links to:

    NCCL::NCCL

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

Call Stack (most recent call first):
  tests/CMakeLists.txt:213 (ConfigureTest)

This is because NCCL::NCCL is specified in tests/CmakeLists.txt

ConfigureTest(
NAME NEIGHBORS_ALL_NEIGHBORS_TEST PATH neighbors/all_neighbors/test_float.cu GPUS 1 PERCENT 100
ADDITIONAL_DEP NCCL::NCCL
)

I could build the unit tests without the NCCL dependency. Is this dependency really necessary even when the --no-mg option is specified?
This PR modifies the CMakeLists.txt file so that the NCCL dependency is set only when the --no-mg option is not specified.

Rel: #785 vis: @jinsolp

@enp1s0 enp1s0 self-assigned this Aug 8, 2025
@enp1s0 enp1s0 requested a review from a team as a code owner August 8, 2025 10:11
@enp1s0 enp1s0 added bug Something isn't working non-breaking Introduces a non-breaking change labels Aug 8, 2025
Copy link
Copy Markdown
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

This can be simplified to:

ConfigureTest(
    NAME NEIGHBORS_ALL_NEIGHBORS_TEST PATH neighbors/all_neighbors/test_float.cu GPUS 1 PERCENT 100
    ADDITIONAL_DEP $<TARGET_NAME_IF_EXISTS:NCCL::NCCL>
  )

Comment thread cpp/tests/CMakeLists.txt Outdated
ConfigureTest(
NAME NEIGHBORS_ALL_NEIGHBORS_TEST PATH neighbors/all_neighbors/test_float.cu GPUS 1 PERCENT 100
ADDITIONAL_DEP NCCL::NCCL
ADDITIONAL_DEP ${NCCL_DEPS}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for finding this! We can just get rid of this NCCL line altogether because this doesn't depend on nccl at all.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thank you @jinsolp for letting me know. I removed the line.

@enp1s0 enp1s0 requested a review from robertmaynard August 18, 2025 02:50
@cjnolet
Copy link
Copy Markdown
Member

cjnolet commented Aug 20, 2025

@robertmaynard mind re-reviewing when you have a moment?

@cjnolet
Copy link
Copy Markdown
Member

cjnolet commented Aug 27, 2025

/merge

@rapids-bot rapids-bot Bot merged commit e02b7d1 into rapidsai:branch-25.10 Aug 28, 2025
104 of 106 checks passed
@enp1s0 enp1s0 deleted the fix-NEIGHBORS_ALL_NEIGHBORS_TEST-build-with-no-mg branch September 29, 2025 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working non-breaking Introduces a non-breaking change

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants