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

Draft: Revert partially constructed segments on-error in segment init function #361

Closed
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
ef06972
Don't skip this test as its perfectly valid and passes, the reference…
dagardner-nv Aug 2, 2023
79d144b
Test for issue #360
dagardner-nv Aug 2, 2023
52968bf
Remove unneeded copy/paste code
dagardner-nv Aug 2, 2023
1178c02
Revert added objects and ingress/egress ports on error in segment ini…
dagardner-nv Aug 2, 2023
ac2853c
Better test name
dagardner-nv Aug 2, 2023
6a26e4f
wip
dagardner-nv Aug 2, 2023
cccef95
Only run iwyu on compilation units actually changed in PR
dagardner-nv Aug 2, 2023
6914da7
Checks require clang
dagardner-nv Aug 2, 2023
dbeeedc
Revert "Checks require clang"
dagardner-nv Aug 2, 2023
faf233c
test [no ci]
dagardner-nv Aug 2, 2023
9fa907c
Don't fetch base branch if it's already set
dagardner-nv Aug 2, 2023
81d094f
Add comment explaining version
dagardner-nv Aug 2, 2023
9d55259
Avoid 15.0.7
dagardner-nv Aug 2, 2023
1eb89f7
revert clang version changes [no ci]
dagardner-nv Aug 3, 2023
c0f8a35
Adopt updated versions of boost and libhwlock this <hand wavy> choose…
dagardner-nv Aug 3, 2023
46ac7b5
Apply fix from mdemoret-nv:mdd_force-stubgen-dev
dagardner-nv Aug 3, 2023
a6244c9
Merge branch 'branch-23.11' of github.com:nv-morpheus/MRC into david-…
dagardner-nv Aug 18, 2023
f285c41
Merge branch 'branch-23.11' into david-inconsistent-pipe [no ci]
dagardner-nv Sep 25, 2023
f6503db
Merge branch 'branch-23.11' into david-inconsistent-pipe
dagardner-nv Sep 25, 2023
0f1bf1c
Merge branch 'david-inconsistent-pipe' of github.com:dagardner-nv/MRC…
dagardner-nv Sep 25, 2023
1cd28f6
IWYU changes
dagardner-nv Sep 25, 2023
743d4d3
Merge branch 'branch-23.11' of github.com:nv-morpheus/MRC into david-…
dagardner-nv Sep 25, 2023
dfa066e
Python repro for issue #360
dagardner-nv Sep 26, 2023
9e9a5f7
Place executor.join under exception handler [no ci]
dagardner-nv Sep 26, 2023
38c0e15
Fix repro test, works if segment2 raises, but fails os segment1 raise…
dagardner-nv Sep 26, 2023
22220a2
Add second test, with exception being thrown in both the first and se…
dagardner-nv Sep 26, 2023
06d28a6
WIP: A bunch of bebug logging that needs to be removed, add a destroy…
dagardner-nv Sep 27, 2023
1d12c2f
WIP: Reset m_owned_edge from release_edge_connection
dagardner-nv Sep 27, 2023
22205c0
WIP: more debug logging than you can shake a stick at [no ci]
dagardner-nv Sep 27, 2023
142e9b8
WIP: tests passing [no ci]
dagardner-nv Sep 28, 2023
a0f65fd
Release edge created in constructor [no ci]
dagardner-nv Sep 28, 2023
2289f42
Remove debug logging
dagardner-nv Sep 28, 2023
5d0d6a9
WIP: DO NOT MERGE
dagardner-nv Sep 28, 2023
7d69c5d
Enable logging in test [no ci]
dagardner-nv Sep 28, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ci/conda/environments/clang_env.yml
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
# SPDX-FileCopyrightText: Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0
#
# Licensed under the Apache License, Version 2.0 (the "License");
# Licensed under the Apache License, Version 2.0 (the License);
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# distributed under the License is distributed on an AS IS BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
Expand Down
4 changes: 2 additions & 2 deletions ci/conda/environments/dev_env.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ dependencies:
- autoconf>=2.69
- bash-completion
- benchmark=1.6.0
- boost-cpp=1.74
- boost-cpp=1.82
- ccache
- cmake=3.24
- cuda-toolkit # Version comes from the channel above
Expand All @@ -46,7 +46,7 @@ dependencies:
- isort
- jinja2=3.0
- lcov=1.15
- libhwloc=2.5
- libhwloc=2.9.2
- libprotobuf=3.21
- librmm=23.06
- libtool
Expand Down
5 changes: 4 additions & 1 deletion ci/release/update-version.sh
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,10 @@ function sed_runner() {

# .gitmodules
git submodule set-branch -b branch-${NEXT_SHORT_TAG} morpheus_utils
git submodule update --remote
if [[ "$(git diff --name-only | grep .gitmodules)" != "" ]]; then
# Only update the submodules if setting the branch changed .gitmodules
git submodule update --remote
fi

# Root CMakeLists.txt
sed_runner 's/'"VERSION ${CURRENT_FULL_VERSION}.*"'/'"VERSION ${NEXT_FULL_VERSION}"'/g' CMakeLists.txt
Expand Down
17 changes: 15 additions & 2 deletions ci/scripts/cpp_checks.sh
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,22 @@ if [[ -n "${MRC_MODIFIED_FILES}" ]]; then

# Include What You Use
if [[ "${SKIP_IWYU}" == "" ]]; then
IWYU_DIRS="cpp python"
# Remove .h, .hpp, and .cu files from the modified list
shopt -s extglob
IWYU_MODIFIED_FILES=( "${MRC_MODIFIED_FILES[@]/*.@(h|hpp|cu)/}" )

# Get the list of compiled files relative to this directory
WORKING_PREFIX="${PWD}/"
COMPILED_FILES=( $(jq -r .[].file ${BUILD_DIR}/compile_commands.json | sort -u ) )
COMPILED_FILES=( "${COMPILED_FILES[@]/#$WORKING_PREFIX/}" )
COMBINED_FILES=("${COMPILED_FILES[@]}")
COMBINED_FILES+=("${IWYU_MODIFIED_FILES[@]}")

# Find the intersection between compiled files and modified files
IWYU_MODIFIED_FILES=( $(printf '%s\0' "${COMBINED_FILES[@]}" | sort -z | uniq -d -z | xargs -0n1) )

NUM_PROC=$(get_num_proc)
IWYU_OUTPUT=`${IWYU_TOOL} -p ${BUILD_DIR} -j ${NUM_PROC} ${IWYU_DIRS} 2>&1`
IWYU_OUTPUT=`${IWYU_TOOL} -p ${BUILD_DIR} -j ${NUM_PROC} ${IWYU_MODIFIED_FILES[@]} 2>&1`
IWYU_RETVAL=$?
fi
else
Expand Down
22 changes: 12 additions & 10 deletions ci/scripts/github/common.sh
Original file line number Diff line number Diff line change
Expand Up @@ -106,16 +106,18 @@ function update_conda_env() {
print_env_vars

function fetch_base_branch() {
rapids-logger "Retrieving base branch from GitHub API"
[[ -n "$GH_TOKEN" ]] && CURL_HEADERS=('-H' "Authorization: token ${GH_TOKEN}")
RESP=$(
curl -s \
-H "Accept: application/vnd.github.v3+json" \
"${CURL_HEADERS[@]}" \
"${GITHUB_API_URL}/repos/${ORG_NAME}/${REPO_NAME}/pulls/${PR_NUM}"
)

BASE_BRANCH=$(echo "${RESP}" | jq -r '.base.ref')
if [[ "${BASE_BRANCH}" == "" ]]; then
rapids-logger "Retrieving base branch from GitHub API"
[[ -n "$GH_TOKEN" ]] && CURL_HEADERS=('-H' "Authorization: token ${GH_TOKEN}")
RESP=$(
curl -s \
-H "Accept: application/vnd.github.v3+json" \
"${CURL_HEADERS[@]}" \
"${GITHUB_API_URL}/repos/${ORG_NAME}/${REPO_NAME}/pulls/${PR_NUM}"
)

BASE_BRANCH=$(echo "${RESP}" | jq -r '.base.ref')
fi

# Change target is the branch name we are merging into but due to the weird way jenkins does
# the checkout it isn't recognized by git without the origin/ prefix
Expand Down
4 changes: 4 additions & 0 deletions cpp/mrc/src/internal/segment/builder_definition.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,10 @@ void BuilderDefinition::initialize()
<< ", Segment Rank: " << m_rank << ". Exception message:\n"
<< e.what();

m_objects.clear();
m_ingress_ports.clear();
m_egress_ports.clear();

// Rethrow after logging
std::rethrow_exception(std::current_exception());
}
Expand Down
48 changes: 46 additions & 2 deletions cpp/mrc/tests/test_pipeline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include <map>
#include <memory>
#include <ostream>
#include <stdexcept>
#include <string>
#include <utility>
#include <vector>
Expand Down Expand Up @@ -105,8 +106,6 @@ TEST_F(TestPipeline, DuplicateSegments)

TEST_F(TestPipeline, TwoSegment)
{
GTEST_SKIP() << "#185";

std::atomic<int> next_count = 0;
std::atomic<int> complete_count = 0;

Expand Down Expand Up @@ -162,6 +161,51 @@ TEST_F(TestPipeline, TwoSegment)
LOG(INFO) << "Done" << std::endl;
}

TEST_F(TestPipeline, SegmentInitErrorHandling)
{
// Test to reproduce issue #360
auto pipeline = mrc::make_pipeline();

auto seg_1 =
pipeline->make_segment("seg_1", segment::EgressPorts<float>({"float_port"}), [](segment::IBuilder& seg) {
auto rx_source = seg.make_source<float>("rx_source", [](rxcpp::subscriber<float> s) {
FAIL() << "This should not be called";
});

auto my_float_egress = seg.get_egress<float>("float_port");

seg.make_edge(rx_source, my_float_egress);
});

auto seg_2 = pipeline->make_segment("seg_2",
segment::IngressPorts<float>({"float_port"}),
[&](segment::IBuilder& seg) {
auto my_float_ingress = seg.get_ingress<float>("float_port");

auto rx_sink = seg.make_sink<float>("rx_sink",
rxcpp::make_observer_dynamic<float>(
[&](float x) {
FAIL() << "This should not be "
"called";
},
[&]() {
FAIL() << "This should not be "
"called";
}));

seg.make_edge(my_float_ingress, rx_sink);
throw std::runtime_error("Error in initializer");
});

Executor exec(std::move(m_options));

exec.register_pipeline(std::move(pipeline));

exec.start();

EXPECT_THROW(exec.join(), std::runtime_error);
}

/*
TEST_F(TestPipeline, TwoSegmentManualTag)
{
Expand Down
2 changes: 1 addition & 1 deletion external/utilities
Submodule utilities updated 1 files
+32 −0 CHANGELOG.md