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

Revert print vector changes because of std::vector<bool> #681

Merged
merged 1 commit into from
May 26, 2022

Conversation

lowener
Copy link
Contributor

@lowener lowener commented May 25, 2022

The specialization of std::vector when T=bool is unfortunately causing compilation issue in cuml because the data() function member is not implemented. And the elements may not be stored contiguously.

(Link to the CI failure: https://gpuci.gpuopenanalytics.com/job/rapidsai/job/gpuci/job/cuml/job/prb/job/cuml-cpu-cuda-build-arm64/CUDA=11.5/1364/console)
cc @achirkin

@lowener lowener requested a review from a team as a code owner May 25, 2022 15:57
@github-actions github-actions bot added the cpp label May 25, 2022
@lowener lowener added bug Something isn't working non-breaking Non-breaking change cpp and removed cpp labels May 25, 2022
cjnolet
cjnolet previously approved these changes May 25, 2022
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.

LGTM

Copy link
Contributor

@achirkin achirkin left a comment

Choose a reason for hiding this comment

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

Huh, good point :) still, let us not have manual allocations

CUDA_CHECK(
cudaMemcpy(host_mem.data(), devMem, componentsCount * sizeof(T), cudaMemcpyDeviceToHost));
print_host_vector(variable_name, host_mem.data(), componentsCount, out);
T* host_mem = new T[componentsCount];
Copy link
Contributor

@achirkin achirkin May 25, 2022

Choose a reason for hiding this comment

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

I'd suggest to use unique_ptr to avoid a memory leak on exception, e.g.

auto host_mem = std::make_unique<T[]>(componentsCount);

@cjnolet cjnolet dismissed their stale review May 25, 2022 20:17

Dismissing from Artem's requested changes

@cjnolet
Copy link
Member

cjnolet commented May 26, 2022

I’m going to go ahead and merge this guy for now to fix the cuml build, though @lowener it would be nice if we could still address @achirkin’s concerns in a follow on PR (maybe for 22.08)

@cjnolet
Copy link
Member

cjnolet commented May 26, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 120d478 into rapidsai:branch-22.06 May 26, 2022
rapids-bot bot pushed a commit that referenced this pull request Jun 1, 2022
Continuation of #681

Authors:
  - Micka (https://github.com/lowener)

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

URL: #695
@lowener lowener deleted the 22.06-fix-print branch August 15, 2022 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cpp non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants