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

Use std::optional for host types #17015

Conversation

robertmaynard
Copy link
Contributor

Description

cuda::std::optional shouldn't be used for host types such as std::vector as it requires the constructors of the T types to be host+device.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@robertmaynard robertmaynard added bug Something isn't working non-breaking Non-breaking change labels Oct 8, 2024
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Oct 8, 2024
Copy link
Contributor

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

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

LGTM!

@robertmaynard robertmaynard marked this pull request as ready for review October 8, 2024 18:51
@robertmaynard robertmaynard requested a review from a team as a code owner October 8, 2024 18:51
@robertmaynard
Copy link
Contributor Author

/merge

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.

Re-approving additional changes.

@bdice
Copy link
Contributor

bdice commented Oct 8, 2024

There are some failures in CI like this:

In file included from $SRC_DIR/cpp/build/_deps/gtest-src/googletest/include/gtest/gtest-printers.h:115,
                 from $SRC_DIR/cpp/build/_deps/gtest-src/googletest/include/gtest/gtest-matchers.h:48,
                 from $SRC_DIR/cpp/build/_deps/gtest-src/googletest/include/gtest/internal/gtest-death-test-internal.h:47,
                 from $SRC_DIR/cpp/build/_deps/gtest-src/googletest/include/gtest/gtest-death-test.h:43,
                 from $SRC_DIR/cpp/build/_deps/gtest-src/googletest/include/gtest/gtest.h:65,
                 from $SRC_DIR/cpp/build/_deps/gtest-src/googlemock/include/gmock/internal/gmock-internal-utils.h:50,
                 from $SRC_DIR/cpp/build/_deps/gtest-src/googlemock/include/gmock/gmock-actions.h:145,
                 from $SRC_DIR/cpp/build/_deps/gtest-src/googlemock/include/gmock/gmock.h:56,
                 from $SRC_DIR/cpp/include/cudf_test/cudf_gtest.hpp:19,
                 from $SRC_DIR/cpp/include/cudf_test/base_fixture.hpp:19,
                 from $SRC_DIR/cpp/tests/io/parquet_common.hpp:19,
                 from $SRC_DIR/cpp/tests/io/parquet_writer_test.cpp:17:
$SRC_DIR/cpp/tests/io/parquet_writer_test.cpp:1033:84: error: invalid initialization of reference of type 'const cuda::std::__4::optional<cudf::io::parquet::detail::ConvertedType>&' from expression of type 'const std::optional<cudf::io::parquet::detail::ConvertedType>'
 1033 |       EXPECT_TRUE(compare_binary(ci.max_values[0], stats.max_value.value(), ptype, ctype) >= 0);
      |                                                                                    ^~~~~
In file included from $SRC_DIR/cpp/tests/io/parquet_writer_test.cpp:17:
$SRC_DIR/cpp/tests/io/parquet_common.hpp:175:93: note: in passing argument 4 of 'int32_t compare_binary(const std::vector<unsigned char>&, const std::vector<unsigned char>&, cudf::io::parquet::detail::Type, const cuda::std::__4::optional<cudf::io::parquet::detail::ConvertedType>&)'
  175 |                        cuda::std::optional<cudf::io::parquet::detail::ConvertedType> const& ctype);
      |                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~

Some additional changes are probably needed.

@robertmaynard
Copy link
Contributor Author

There are some failures in CI like this:

In file included from $SRC_DIR/cpp/build/_deps/gtest-src/googletest/include/gtest/gtest-printers.h:115,
                 from $SRC_DIR/cpp/build/_deps/gtest-src/googletest/include/gtest/gtest-matchers.h:48,
                 from $SRC_DIR/cpp/build/_deps/gtest-src/googletest/include/gtest/internal/gtest-death-test-internal.h:47,
                 from $SRC_DIR/cpp/build/_deps/gtest-src/googletest/include/gtest/gtest-death-test.h:43,
                 from $SRC_DIR/cpp/build/_deps/gtest-src/googletest/include/gtest/gtest.h:65,
                 from $SRC_DIR/cpp/build/_deps/gtest-src/googlemock/include/gmock/internal/gmock-internal-utils.h:50,
                 from $SRC_DIR/cpp/build/_deps/gtest-src/googlemock/include/gmock/gmock-actions.h:145,
                 from $SRC_DIR/cpp/build/_deps/gtest-src/googlemock/include/gmock/gmock.h:56,
                 from $SRC_DIR/cpp/include/cudf_test/cudf_gtest.hpp:19,
                 from $SRC_DIR/cpp/include/cudf_test/base_fixture.hpp:19,
                 from $SRC_DIR/cpp/tests/io/parquet_common.hpp:19,
                 from $SRC_DIR/cpp/tests/io/parquet_writer_test.cpp:17:
$SRC_DIR/cpp/tests/io/parquet_writer_test.cpp:1033:84: error: invalid initialization of reference of type 'const cuda::std::__4::optional<cudf::io::parquet::detail::ConvertedType>&' from expression of type 'const std::optional<cudf::io::parquet::detail::ConvertedType>'
 1033 |       EXPECT_TRUE(compare_binary(ci.max_values[0], stats.max_value.value(), ptype, ctype) >= 0);
      |                                                                                    ^~~~~
In file included from $SRC_DIR/cpp/tests/io/parquet_writer_test.cpp:17:
$SRC_DIR/cpp/tests/io/parquet_common.hpp:175:93: note: in passing argument 4 of 'int32_t compare_binary(const std::vector<unsigned char>&, const std::vector<unsigned char>&, cudf::io::parquet::detail::Type, const cuda::std::__4::optional<cudf::io::parquet::detail::ConvertedType>&)'
  175 |                        cuda::std::optional<cudf::io::parquet::detail::ConvertedType> const& ctype);
      |                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~

Some additional changes are probably needed.

Thanks. Found the function that needed updating to std::optional

@rapids-bot rapids-bot bot merged commit dfdae59 into rapidsai:branch-24.12 Oct 9, 2024
101 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants