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 partially constructed segments on-error in segment init function #434

Open
wants to merge 31 commits into
base: branch-24.10
Choose a base branch
from

Conversation

dagardner-nv
Copy link
Contributor

@dagardner-nv dagardner-nv commented Jan 19, 2024

Description

  • Explicitly release edges from ingress/egress ports and manifolds.
  • Ensure that the inner_edge held by the ForwardingEgressProvider is disconnected.
  • Update ~EdgeHolder to LOG(FATAL) in debug mode, LOG(ERROR) otherwise. The thinking here is that this error is only of use to MRC developers and not of use to downstream users.

Closes #360

Checklist

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

@dagardner-nv dagardner-nv requested review from a team as code owners January 19, 2024 22:26
@dagardner-nv dagardner-nv self-assigned this Jan 19, 2024
@dagardner-nv dagardner-nv added bug Something isn't working non-breaking Non-breaking change labels Jan 19, 2024
@dagardner-nv dagardner-nv marked this pull request as draft January 20, 2024 00:01
@dagardner-nv dagardner-nv added the skip-ci Optionally Skip CI for this PR label Jan 20, 2024
…n loaded and added to the segment [no ci]
…odules.[c|h]pp and are not referenced in any cmake scripts. Likely these were just forgotten to be deleted in a previous refactor [no ci]
…y only of use to MRC developers and is of little use to downstream users [no ci]
…al in debug builds, add a regex match for the specific error
…de or a thrown exception in release mode, update existing tests which are currently triggering a LOG(FATAL) in debug mode
@dagardner-nv dagardner-nv removed the skip-ci Optionally Skip CI for this PR label Jan 23, 2024
@dagardner-nv dagardner-nv marked this pull request as ready for review January 23, 2024 23:44
@dagardner-nv dagardner-nv requested a review from a team as a code owner January 23, 2024 23:44
cpp/mrc/include/mrc/manifold/interface.hpp Outdated Show resolved Hide resolved
if (!(state->m_is_destroyed))
{
// Only call the on_complete if we have been connected and `this` is still alive
this->on_complete();
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to see if there are any implementations of on_complete. If there arent, then we should just scrap the disconnector and remove the state and mutex changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked, the classes which implement on_complete are:

  • mrc::node::NodeComponent Calls SourceProperties<OutputT>::release_edge_connection(); and do_on_complete which is unimpl in the base
  • mrc::node::RouterBase calls MultiSourceProperties<KeyT, output_data_t>::release_edge_connections();

do_on_complete in turn is used by LambdaNodeComponent to call m_on_complete_fn if defined.

cpp/mrc/src/internal/pipeline/pipeline_instance.hpp Outdated Show resolved Hide resolved
@mdemoret-nv mdemoret-nv changed the base branch from branch-24.03 to branch-24.06 April 5, 2024 23:53
@dagardner-nv dagardner-nv marked this pull request as draft April 10, 2024 19:52
@dagardner-nv dagardner-nv marked this pull request as ready for review April 12, 2024 16:54
@dagardner-nv dagardner-nv changed the base branch from branch-24.06 to branch-24.10 August 1, 2024 21:31
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

Attention: Patch coverage is 72.82609% with 25 lines in your changes missing coverage. Please review.

Project coverage is 74.16%. Comparing base (bceb7ef) to head (6040afb).
Report is 7 commits behind head on branch-24.10.

Files with missing lines Patch % Lines
cpp/mrc/include/mrc/edge/edge_holder.hpp 25.00% 12 Missing ⚠️
...pp/mrc/src/internal/pipeline/pipeline_instance.cpp 78.94% 4 Missing ⚠️
cpp/mrc/include/mrc/manifold/egress.hpp 50.00% 2 Missing ⚠️
cpp/mrc/include/mrc/manifold/ingress.hpp 50.00% 2 Missing ⚠️
cpp/mrc/include/mrc/node/source_properties.hpp 93.33% 1 Missing ⚠️
cpp/mrc/include/mrc/segment/ingress_port.hpp 75.00% 1 Missing ⚠️
cpp/mrc/include/mrc/segment/object.hpp 0.00% 1 Missing ⚠️
cpp/mrc/include/mrc/segment/runnable.hpp 66.66% 1 Missing ⚠️
cpp/mrc/src/public/manifold/manifold.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-24.10     #434      +/-   ##
================================================
+ Coverage         74.07%   74.16%   +0.09%     
================================================
  Files               403      403              
  Lines             14401    14471      +70     
  Branches           1132     1137       +5     
================================================
+ Hits              10668    10733      +65     
- Misses             3733     3738       +5     
Flag Coverage Δ
cpp 70.90% <70.65%> (+0.05%) ⬆️
py 42.83% <66.30%> (+1.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...pp/mrc/include/mrc/manifold/composite_manifold.hpp 88.88% <100.00%> (+2.22%) ⬆️
cpp/mrc/include/mrc/manifold/interface.hpp 100.00% <ø> (ø)
cpp/mrc/include/mrc/node/queue.hpp 100.00% <100.00%> (ø)
cpp/mrc/include/mrc/node/rx_node.hpp 93.47% <ø> (ø)
cpp/mrc/include/mrc/segment/component.hpp 100.00% <100.00%> (ø)
...pp/mrc/src/internal/segment/builder_definition.cpp 84.11% <100.00%> (+0.88%) ⬆️
cpp/mrc/src/internal/segment/segment_instance.cpp 84.12% <100.00%> (+0.52%) ⬆️
cpp/mrc/include/mrc/node/source_properties.hpp 77.55% <93.33%> (+2.55%) ⬆️
cpp/mrc/include/mrc/segment/ingress_port.hpp 77.27% <75.00%> (-0.51%) ⬇️
cpp/mrc/include/mrc/segment/object.hpp 50.96% <0.00%> (-0.50%) ⬇️
... and 6 more

... and 5 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bceb7ef...6040afb. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Non-breaking change
Projects
Status: Review - Ready for Review
Development

Successfully merging this pull request may close these issues.

[BUG]: Pipeline left in an inconsistent state when the init function passed to make_segment throws
2 participants