Skip to content

Conversation

@mbrobbel
Copy link
Member

@mbrobbel mbrobbel commented Feb 2, 2022

  • Set ARROW_WITH_OPENTELEMETRY: ON in C++ GitHub Actions workflow
  • Add default value for ExecNode::ToStringExtra argument indent argument

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

@mbrobbel mbrobbel changed the title ARROW-15533: [C++] ARROW_WITH_OPENTELEMETRY is not checked in CI ARROW-15533: [C++] Check ARROW_WITH_OPENTELEMETRY in CI Feb 2, 2022
@wjones127
Copy link
Member

Is the option making it though to the build? I see the env variable but this line suggests it's not actually turned on in the CMake Configure:

https://github.com/apache/arrow/runs/5040342323?check_suite_focus=true#step:7:460

Also, maybe we should also add this to a Linux build, like the AMD64 Conda C++ one?

@mbrobbel
Copy link
Member Author

mbrobbel commented Feb 3, 2022

Is the option making it though to the build? I see the env variable but this line suggests it's not actually turned on in the CMake Configure:

https://github.com/apache/arrow/runs/5040342323?check_suite_focus=true#step:7:460

Thanks for spotting that! I forgot to update the build script.

Also, maybe we should also add this to a Linux build, like the AMD64 Conda C++ one?

This is blocked on #11963. I'll add a reminder to that PR to enable OpenTelemetry in CI once that gets merged.

@mbrobbel
Copy link
Member Author

mbrobbel commented Feb 3, 2022

Disabling OT in Windows builds in this PR. The fix from #11963 should also fix those builds: https://github.com/apache/arrow/blob/8573ba86990968ab95ac8e53d05071f917da93d4/cpp/cmake_modules/ThirdpartyToolchain.cmake#L4126-L4128

@pitrou
Copy link
Member

pitrou commented Feb 3, 2022

@github-actions crossbow submit -g cpp

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

Revision: 02edcb8

Submitted crossbow builds: ursacomputing/crossbow @ actions-1572

Task Status
test-build-cpp-fuzz Github Actions
test-conda-cpp Github Actions
test-conda-cpp-valgrind Azure
test-debian-10-cpp-amd64 Github Actions
test-debian-10-cpp-i386 Github Actions
test-debian-11-cpp-amd64 Github Actions
test-debian-11-cpp-i386 Github Actions
test-fedora-33-cpp Github Actions
test-ubuntu-18.04-cpp Github Actions
test-ubuntu-18.04-cpp-release Github Actions
test-ubuntu-18.04-cpp-static Github Actions
test-ubuntu-20.04-cpp Github Actions
test-ubuntu-20.04-cpp-14 Github Actions
test-ubuntu-20.04-cpp-17 Github Actions
test-ubuntu-20.04-cpp-bundled Github Actions
test-ubuntu-20.04-cpp-thread-sanitizer Github Actions
test-ubuntu-21.04-cpp Github Actions

@pitrou
Copy link
Member

pitrou commented Feb 3, 2022

@github-actions crossbow submit test-ubuntu-18.04-*

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

+1, let's wait for CI

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

Revision: 105b3c0

Submitted crossbow builds: ursacomputing/crossbow @ actions-1573

Task Status
test-ubuntu-18.04-cpp Github Actions
test-ubuntu-18.04-cpp-release Github Actions
test-ubuntu-18.04-cpp-static Github Actions
test-ubuntu-18.04-r-sanitizer Azure

@pitrou pitrou closed this in 7d34a7c Feb 3, 2022
@mbrobbel mbrobbel deleted the arrow-15533 branch February 3, 2022 13:24
@ursabot
Copy link

ursabot commented Feb 3, 2022

Benchmark runs are scheduled for baseline = 56d060c and contender = 7d34a7c. 7d34a7c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.39% ⬆️0.78%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@jorisvandenbossche
Copy link
Member

The "python-sdist" nightly build started failing (see https://github.com/ursacomputing/crossbow/runs/5062761297?check_suite_focus=true), with a message that seems related to OpenTelemetry:

 -- stderr output is:
CMake Error at /usr/share/cmake-3.16/Modules/ExternalProject.cmake:2421 (message):
  error: could not find git for clone of opentelemetry-proto
Call Stack (most recent call first):
  /usr/share/cmake-3.16/Modules/ExternalProject.cmake:3236 (_ep_add_download_command)
  cmake/opentelemetry-proto.cmake:9 (ExternalProject_Add)
  CMakeLists.txt:363 (include)



CMake Error at /build/cpp/opentelemetry_ep-prefix/src/opentelemetry_ep-stamp/opentelemetry_ep-configure-DEBUG.cmake:47 (message):
  Stopping after outputting logs.

So potentially related to this PR (note, I didn't actually look into it, just looked at commits in the last day that could be related to the new failure, and pinging here in case you could take a look).

@mbrobbel
Copy link
Member Author

mbrobbel commented Feb 4, 2022

The "python-sdist" nightly build started failing (see https://github.com/ursacomputing/crossbow/runs/5062761297?check_suite_focus=true), with a message that seems related to OpenTelemetry:

 -- stderr output is:
CMake Error at /usr/share/cmake-3.16/Modules/ExternalProject.cmake:2421 (message):
  error: could not find git for clone of opentelemetry-proto
Call Stack (most recent call first):
  /usr/share/cmake-3.16/Modules/ExternalProject.cmake:3236 (_ep_add_download_command)
  cmake/opentelemetry-proto.cmake:9 (ExternalProject_Add)
  CMakeLists.txt:363 (include)



CMake Error at /build/cpp/opentelemetry_ep-prefix/src/opentelemetry_ep-stamp/opentelemetry_ep-configure-DEBUG.cmake:47 (message):
  Stopping after outputting logs.

So potentially related to this PR (note, I didn't actually look into it, just looked at commits in the last day that could be related to the new failure, and pinging here in case you could take a look).

It looks like git is removed before the build starts: https://github.com/ursacomputing/crossbow/runs/5062761297?check_suite_focus=true#step:6:501.
Should we change that or disable OpenTelemetry for these builds?

@jorisvandenbossche
Copy link
Member

This removal of git was added in #10342, cc @kszucs

@pitrou
Copy link
Member

pitrou commented Feb 7, 2022

I opened #12356 for the python-sdist failure.

@jorisvandenbossche
Copy link
Member

This removal of git was added in #10342, cc @kszucs

@kszucs do you remember the reason for it?
We now have a similar failure of the python-sdist build, this time the new substrait option requiring git:

 CMake Error at /usr/share/cmake-3.16/Modules/ExternalProject.cmake:2421 (message):
  error: could not find git for clone of substrait_ep
Call Stack (most recent call first):
  /usr/share/cmake-3.16/Modules/ExternalProject.cmake:3236 (_ep_add_download_command)
  src/arrow/engine/CMakeLists.txt:50 (externalproject_add)

@kszucs
Copy link
Member

kszucs commented Feb 23, 2022

This removal of git was added in #10342, cc @kszucs

@kszucs do you remember the reason for it?

Git was used to generate a version number even for a python sdist release with explicit version and already available source code.

We now have a similar failure of the python-sdist build, this time the new substrait option requiring git:

 CMake Error at /usr/share/cmake-3.16/Modules/ExternalProject.cmake:2421 (message):
  error: could not find git for clone of substrait_ep
Call Stack (most recent call first):
  /usr/share/cmake-3.16/Modules/ExternalProject.cmake:3236 (_ep_add_download_command)
  src/arrow/engine/CMakeLists.txt:50 (externalproject_add)

I think we should avoid hard dependency on git and prefer downloading tarballs from github.

@jorisvandenbossche
Copy link
Member

I think we should avoid hard dependency on git and prefer downloading tarballs from github.

Opened a JIRA for this: https://issues.apache.org/jira/browse/ARROW-15760

@kszucs
Copy link
Member

kszucs commented Feb 23, 2022

We should pass

    URL "https://github.com/${ARROW_SUBSTRAIT_REPO}/archive/${ARROW_SUBSTRAIT_TAG}.tar.gz"

instead of

GIT_REPOSITORY "${ARROW_SUBSTRAIT_REPO}"
GIT_TAG "${ARROW_SUBSTRAIT_TAG}"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants