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

[BUG] RAFT header conflicts #128

Closed
bryevdv opened this issue Jan 27, 2021 · 8 comments · Fixed by #378
Closed

[BUG] RAFT header conflicts #128

bryevdv opened this issue Jan 27, 2021 · 8 comments · Fixed by #378
Labels
bug Something isn't working

Comments

@bryevdv
Copy link

bryevdv commented Jan 27, 2021

Describe the bug
RAFT headers do not play nicely with some other RAPIDS libraries because of duplicated names

in rapids-js, just doing

include <cugraph/graph.hpp>

results in:

./../../.cache/cpm/raft/39a4ee56e5ca66c2be3eb2b78767b9d0d46759cc/cpp/include/raft/cudart_utils.h:52: error: "CUDA_TRY" redefined [-Werror]
   52 | #define CUDA_TRY(call)                                                        \
      | 
In file included from ../../../cudf/src/node_cudf/utilities/error.hpp:20,
                 from ../../../cudf/src/node_cudf/scalar.hpp:17,
                 from ../../../cudf/src/node_cudf/column.hpp:17,
                 from ../../src/node_cugraph/graph_coo.hpp:17,
                 from ../../src/graph_coo.cpp:15:
../../../.cache/cpm/cudf/6bbbff12b33dfcdeef951b16babffd1472c03bdb/cpp/include/cudf/utilities/error.hpp:102: note: this is the location of the previous definition
  102 | #define CUDA_TRY(call)                                            \
      | 

Currently to mitigate we have an internal/graph.hpp header to do this:

#ifdef CUDA_TRY
#undef CUDA_TRY
#endif
#ifdef CHECK_CUDA
#undef CHECK_CUDA
#endif
#include <cugraph/graph.hpp>
#ifdef CHECK_CUDA
#undef CHECK_CUDA
#endif
#ifdef CUDA_TRY
#undef CUDA_TRY
#endif

But it would be better if these RAPIDS tools could coordinate to avoid these clashes.

cc @trxcllnt

@bryevdv bryevdv added the bug Something isn't working label Jan 27, 2021
@github-actions
Copy link

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@github-actions
Copy link

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

@bryevdv
Copy link
Author

bryevdv commented May 27, 2021

This still seems worth addressing.

@AjayThorve
Copy link
Member

+1 Facing similar issues while building raft with cuml, for node-rapids bindings.

@cjnolet
Copy link
Member

cjnolet commented Nov 1, 2021

@bryevdv @AjayThorve @trxclln

tIs the problem here is related to conflicting names which are defined in multiple projects or are these conflicts defined only in RAFT but having problems because somehow the RAFT headers are being installed in different locations? If it's the former, I would recommend we centralize the conflicting names into RAFT (potentially prefixed w/ a RAFT specifier) to remove duplication. If the latter, we might want to consider consolidating the RAFT header installs. Any thoughts?

@bryevdv
Copy link
Author

bryevdv commented Nov 1, 2021

cc @trxcllnt

@cjnolet I assume the former. E.g. CUDA_TRY appears to be defined here in Raft:

#define CUDA_TRY(call) \

and here in cudf

https://github.com/rapidsai/cudf/blob/237b0ce8645bf210db791b86417781d589096d6b/cpp/include/cudf/utilities/error.hpp#L102

@jlowe
Copy link
Member

jlowe commented Nov 11, 2021

Ran into the same problem described by @bryevdv trying to use RAFT and cuDF APIs in a compilation unit.

@cjnolet
Copy link
Member

cjnolet commented Nov 15, 2021

I will be creating a PR for this shortly, though we've agreed to add the new macros and deprecate the old ones rather than renaming. In the meantime, I will wrap the current functions w/ conditionals to test if it's been defined, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants