DAQIRI Engine Rename#1615
Open
cliffburdick wants to merge 4 commits into
Open
Conversation
DAQIRI renamed its manager/backend abstraction to 'engine' (ManagerType->EngineType, get_manager_type->get_engine_type, manager_type_to_string->engine_type_to_string, DAQIRI_MGR->DAQIRI_ENGINE). Update the ported networking examples to match: - daqiri_raw_ethernet_bench: drop the unnecessary runtime engine-type check (the raw Ethernet bench only runs on the DPDK engine) and build the RX/TX operators directly under DAQIRI_ENGINE_DPDK; rename the DAQIRI_MGR build option to DAQIRI_ENGINE and DAQIRI_MANAGER_LIST to DAQIRI_ENGINE_LIST (CMake + pytest helpers). - network_radar_pipeline: use engine_type_to_string(common_.engine_type) in the DAQIRI TX connector. - HoloHubDaqiri.cmake and the standalone CMakeLists.txt.install files: rename the manager terminology to engine. Signed-off-by: Cliff Burdick <cburdick@nvidia.com>
The DAQIRI engine rename also moved the socket transport into the endpoint URI scheme: the top-level 'protocol:' cfg field is now rejected as legacy, and socket_config takes 'local_addr'/'remote_addr' URIs (udp://host:port) instead of local_ip/local_port/remote_ip/remote_port. Convert the socket-based examples to the new layout: - daqiri_socket_ping - simple_pdw_pipeline - velodyne_lidar_app (lidar.yaml) Signed-off-by: Cliff Burdick <cburdick@nvidia.com>
cliffburdick
force-pushed
the
daqiri-engine-rename
branch
from
27d12d3 to
92b710b
Compare
Contributor
Greptile SummaryThis PR performs a wholesale rename of DAQIRI's "manager" terminology to "engine" across build scripts, C++ source, Python tests, and YAML configs, tracking an upstream DAQIRI library API change. It also migrates three socket YAML configs to the new URI-style address format (
Confidence Score: 5/5Purely a terminology rename and upstream API migration — no logic changes beyond removing a now-redundant runtime branch in main.cpp. All changed files are self-consistent: CMake variables, compile definitions, C++ preprocessor guards, Python test fixtures, environment variable names, and YAML config keys all move from manager to engine in lockstep. The YAML URI consolidation maps one-to-one to the previous separate fields with no address or port values changed. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph "Old behaviour (main.cpp)"
A1[compose] --> B1["daqiri::get_manager_type() — runtime"]
B1 --> C1{ManagerType == DPDK?}
C1 -- Yes --> D1{"#if DAQIRI_MGR_DPDK — compile-time"}
D1 -- defined --> E1[Add RX/TX operators]
D1 -- not defined --> F1[LOG_ERROR + exit]
C1 -- No --> G1[LOG_ERROR Unsupported + exit]
end
subgraph "New behaviour (main.cpp)"
A2[compose] --> B2{"#if DAQIRI_ENGINE_DPDK — compile-time only"}
B2 -- defined --> C2[Add RX/TX operators]
B2 -- not defined --> D2["LOG_ERROR: rebuild with -DDAQIRI_ENGINE=dpdk + exit"]
end
subgraph "YAML socket config migration"
Y1["local_ip + local_port
remote_ip + remote_port
protocol: udp"] -->|replaced by| Y2["local_addr: udp://host:port
remote_addr: udp://host:port"]
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
subgraph "Old behaviour (main.cpp)"
A1[compose] --> B1["daqiri::get_manager_type() — runtime"]
B1 --> C1{ManagerType == DPDK?}
C1 -- Yes --> D1{"#if DAQIRI_MGR_DPDK — compile-time"}
D1 -- defined --> E1[Add RX/TX operators]
D1 -- not defined --> F1[LOG_ERROR + exit]
C1 -- No --> G1[LOG_ERROR Unsupported + exit]
end
subgraph "New behaviour (main.cpp)"
A2[compose] --> B2{"#if DAQIRI_ENGINE_DPDK — compile-time only"}
B2 -- defined --> C2[Add RX/TX operators]
B2 -- not defined --> D2["LOG_ERROR: rebuild with -DDAQIRI_ENGINE=dpdk + exit"]
end
subgraph "YAML socket config migration"
Y1["local_ip + local_port
remote_ip + remote_port
protocol: udp"] -->|replaced by| Y2["local_addr: udp://host:port
remote_addr: udp://host:port"]
end
Reviews (3): Last reviewed commit: "Merge branch 'main' into daqiri-engine-r..." | Re-trigger Greptile |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughThis PR refactors DAQIRI references from "manager" to "engine": build scripts now emit DAQIRI_ENGINE_* compile defs and DAQIRI_ENGINE_LIST for tests; C++ code uses compile-time engine guards and renames members; Python tests use engine-based parametrization; YAMLs consolidate UDP address fields and use stream_type: socket. ChangesDAQIRI Manager-to-Engine Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@applications/daqiri_raw_ethernet_bench/cpp/CMakeLists.txt`:
- Line 120: The CTest ENVIRONMENT line is passing the CMake list
DAQIRI_ENGINE_LIST directly which lets semicolons break into separate env
entries; to fix, join the list into a single string before exporting (e.g., use
list(JOIN DAQIRI_ENGINE_LIST " " DAQIRI_ENGINE_LIST_STR) and then set
ENVIRONMENT "DAQIRI_ENGINE_LIST=${DAQIRI_ENGINE_LIST_STR}" in the
set_tests_properties call) or alternatively update the Python test that reads
os.environ.get("DAQIRI_ENGINE_LIST") to split on both semicolons and whitespace
(e.g., split on re.split(r"[;\s]+", ...)) so multi-engine values like
"dpdk;socket" are preserved as a single environment variable.
In `@applications/daqiri_raw_ethernet_bench/cpp/main.cpp`:
- Around line 50-53: The compose() function currently calls exit(1) when
DAQIRI_ENGINE_DPDK is not enabled which bypasses later cleanup (daqiri_init(),
app->run(), daqiri::print_stats(), daqiri::shutdown()); move the
DAQIRI_ENGINE_DPDK capability check out of compose() and perform it early in
main() before calling daqiri_init(), or replace the exit call with throwing an
exception so upstream callers can run cleanup handlers; update callers and
error-handling in main() to either abort before initialization or catch the
exception and ensure daqiri::shutdown()/print_stats() and app->run() behavior
remain consistent (reference compose(), main(), daqiri_init(),
daqiri::shutdown(), daqiri::print_stats(), and app->run()).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca6774d4-4639-4e7a-8288-817ce04fc34b
📒 Files selected for processing (12)
applications/daqiri_raw_ethernet_bench/cpp/CMakeLists.txtapplications/daqiri_raw_ethernet_bench/cpp/CMakeLists.txt.installapplications/daqiri_raw_ethernet_bench/cpp/main.cppapplications/daqiri_raw_ethernet_bench/cpp/testing/benchmark_utils.pyapplications/daqiri_raw_ethernet_bench/cpp/testing/test_daqiri_raw_ethernet_bench.pyapplications/daqiri_socket_ping/cpp/CMakeLists.txt.installapplications/daqiri_socket_ping/daqiri_socket_ping.yamlapplications/network_radar_pipeline/cpp/daqiri_connectors/adv_networking_tx.cuapplications/network_radar_pipeline/cpp/daqiri_connectors/adv_networking_tx.happlications/simple_pdw_pipeline/simple_pdw_pipeline.yamlapplications/velodyne_lidar_app/cpp/lidar.yamlcmake/HoloHubDaqiri.cmake
| add_python_tests( | ||
| INPUT "testing" | ||
| ENVIRONMENT "DAQIRI_MANAGER_LIST=${DAQIRI_MGR_LIST}" | ||
| ENVIRONMENT "DAQIRI_ENGINE_LIST=${DAQIRI_ENGINE_LIST}" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the Python test's environment variable parsing matches CMake's list format
# 1. Check how CMake list is passed to environment
rg -n "DAQIRI_ENGINE_LIST" applications/daqiri_raw_ethernet_bench/cpp/CMakeLists.txt -A2 -B2
# 2. Check how Python parses the environment variable
rg -n "DAQIRI_ENGINE_LIST" applications/daqiri_raw_ethernet_bench/cpp/testing/test_daqiri_raw_ethernet_bench.py -A2 -B2
# 3. Look for any list(JOIN) operations that might convert format
rg -n "list\(JOIN.*DAQIRI_ENGINE" applications/daqiri_raw_ethernet_bench/cpp/Repository: nvidia-holoscan/holohub
Length of output: 1246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where add_python_tests is defined/used
rg -n "function\\(add_python_tests\\)|macro\\(add_python_tests\\)|add_python_tests\\(" -S . -g'*.cmake' -g'*.txt'
# Inspect the definition(s) around ENVIRONMENT handling
# (Limit output to the most relevant sections)
rg -n "ENVIRONMENT" -S . -g'*.cmake' | head -n 50Repository: nvidia-holoscan/holohub
Length of output: 903
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="cmake/modules/add_python_tests.cmake"
nl -ba "$FILE" | sed -n '1,220p'Repository: nvidia-holoscan/holohub
Length of output: 110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="cmake/modules/add_python_tests.cmake"
cat -n "$FILE" | sed -n '1,220p'Repository: nvidia-holoscan/holohub
Length of output: 7079
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant CMake block (including separate_arguments and call to add_python_tests)
cat -n applications/daqiri_raw_ethernet_bench/cpp/CMakeLists.txt | sed -n '35,140p'
# Show relevant Python test block(s)
cat -n applications/daqiri_raw_ethernet_bench/cpp/testing/test_daqiri_raw_ethernet_bench.py | sed -n '1,140p'Repository: nvidia-holoscan/holohub
Length of output: 9661
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "add_python_tests\\([\\s\\S]*ENVIRONMENT" applications operators cmake -g'*.cmake'Repository: nvidia-holoscan/holohub
Length of output: 49
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "add_python_tests\\(" -S applications operators cmake -g'*.cmake'
rg -n "\\bENVIRONMENT\\b" applications operators cmake -g'*.cmake' | head -n 200Repository: nvidia-holoscan/holohub
Length of output: 321
Fix CMake/CTest ENVIRONMENT formatting for DAQIRI_ENGINE_LIST (semicolon vs whitespace)
DAQIRI_ENGINE_LIST is built as a CMake list (separate_arguments(...)), but it’s passed to CTest as ENVIRONMENT "DAQIRI_ENGINE_LIST=${DAQIRI_ENGINE_LIST}". CTest/set_tests_properties(ENVIRONMENT ...) treats semicolons as separators between environment variable entries, so multi-engine values like dpdk;socket won’t reach Python as a single DAQIRI_ENGINE_LIST string. The Python test then does os.environ.get(...).split() (whitespace), so it won’t parse semicolon-delimited engines correctly. This isn’t currently exercised because the test parametrizes engine as ["dpdk"], but it will break as soon as other engines are added.
Fix by joining the CMake list before exporting (e.g., list(JOIN DAQIRI_ENGINE_LIST_STR " " DAQIRI_ENGINE_LIST) then ENVIRONMENT "DAQIRI_ENGINE_LIST=${DAQIRI_ENGINE_LIST_STR}") or by updating Python to split on both ; and whitespace.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@applications/daqiri_raw_ethernet_bench/cpp/CMakeLists.txt` at line 120, The
CTest ENVIRONMENT line is passing the CMake list DAQIRI_ENGINE_LIST directly
which lets semicolons break into separate env entries; to fix, join the list
into a single string before exporting (e.g., use list(JOIN DAQIRI_ENGINE_LIST "
" DAQIRI_ENGINE_LIST_STR) and then set ENVIRONMENT
"DAQIRI_ENGINE_LIST=${DAQIRI_ENGINE_LIST_STR}" in the set_tests_properties call)
or alternatively update the Python test that reads
os.environ.get("DAQIRI_ENGINE_LIST") to split on both semicolons and whitespace
(e.g., split on re.split(r"[;\s]+", ...)) so multi-engine values like
"dpdk;socket" are preserved as a single environment variable.
Comment on lines
50
to
53
| #else | ||
| HOLOSCAN_LOG_ERROR("DPDK manager/backend is disabled"); | ||
| exit(1); | ||
| HOLOSCAN_LOG_ERROR("DPDK engine is disabled; rebuild with -DDAQIRI_ENGINE=dpdk"); | ||
| exit(1); | ||
| #endif |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Calling exit(1) in compose() bypasses cleanup and may leak resources.
compose() is invoked after daqiri_init() (line 71) has allocated resources. Calling exit(1) here skips app->run(), daqiri::print_stats(), and daqiri::shutdown() (lines 81-82), potentially leaving DAQIRI in an inconsistent state.
Move the DAQIRI_ENGINE_DPDK check earlier in main() before daqiri_init(), or use an exception-based approach so cleanup handlers run.
🔄 Proposed fix: validate engine support before initialization
int main(int argc, char** argv) {
using namespace holoscan;
+
+#if !DAQIRI_ENGINE_DPDK
+ HOLOSCAN_LOG_ERROR("DPDK engine is disabled; rebuild with -DDAQIRI_ENGINE=dpdk");
+ return 1;
+#endif
+
auto app = make_application<App>();
// Get the configurationAnd remove the #else block from compose():
void compose() override {
using namespace holoscan;
const auto [rx_en, tx_en] = daqiri::get_rx_tx_configs_enabled(config());
// The raw Ethernet benchmark only runs on the DPDK engine, so there is no
// need to query or branch on the engine type at runtime.
`#if` DAQIRI_ENGINE_DPDK
if (rx_en) {
auto bench_rx = make_operator<ops::DaqiriRawEthernetBenchDefaultRxOp>(
"bench_rx",
from_config("bench_rx"),
make_condition<BooleanCondition>("is_alive", true));
add_operator(bench_rx);
}
if (tx_en) {
auto bench_tx = make_operator<ops::DaqiriRawEthernetBenchDefaultTxOp>(
"bench_tx",
from_config("bench_tx"),
make_condition<BooleanCondition>("is_alive", true));
add_operator(bench_tx);
}
-#else
- HOLOSCAN_LOG_ERROR("DPDK engine is disabled; rebuild with -DDAQIRI_ENGINE=dpdk");
- exit(1);
`#endif`
}🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 53-53: There is an unknown macro here somewhere. Configuration is required. If HOLOSCAN_OPERATOR_FORWARD_ARGS is a macro then please configure it.
(unknownMacro)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@applications/daqiri_raw_ethernet_bench/cpp/main.cpp` around lines 50 - 53,
The compose() function currently calls exit(1) when DAQIRI_ENGINE_DPDK is not
enabled which bypasses later cleanup (daqiri_init(), app->run(),
daqiri::print_stats(), daqiri::shutdown()); move the DAQIRI_ENGINE_DPDK
capability check out of compose() and perform it early in main() before calling
daqiri_init(), or replace the exit call with throwing an exception so upstream
callers can run cleanup handlers; update callers and error-handling in main() to
either abort before initialization or catch the exception and ensure
daqiri::shutdown()/print_stats() and app->run() behavior remain consistent
(reference compose(), main(), daqiri_init(), daqiri::shutdown(),
daqiri::print_stats(), and app->run()).
tbirdso
approved these changes
Jun 22, 2026
tbirdso
left a comment
tbirdso
left a comment
Contributor
There was a problem hiding this comment.
LGTM, please address AI reviewer comments to merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DAQIRI changed the "manager" terminology to be "engine" now, along with a couple small config changes. This PR addresses them.
Summary by CodeRabbit