Skip to content

feat(platform): add configuration types, module layer, and host control plane#53

Open
lterrac wants to merge 5 commits into
mainfrom
feat/platform-config
Open

feat(platform): add configuration types, module layer, and host control plane#53
lterrac wants to merge 5 commits into
mainfrom
feat/platform-config

Conversation

@lterrac

@lterrac lterrac commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

First-party C++ platform management layer for PyPTO Serving. This PR establishes the host-side control plane that handles distributed deployment, channel lifecycle, and module orchestration — without sitting in the per-token execution hot path.

What is in this PR

Configuration (include/modules/configuration/)

  • Deployment, Partition, Task, Edge, Replica, RequestManager with JSON serialization and Deployment::verify() for topology validation

Modules (include/modules/)

  • broadcastDeployment: deployer broadcasts deployment JSON to all worker ranks over RPC at init time
  • channelController: desired-vs-actual reconciliation loop; creates and tears down SPSC channels as subscriptions change
  • service: wraps taskr::Runtime to drive cooperative background services through the module lifecycle

System (include/system/)

  • Engine: owns module instances, drives the initialize→run→terminate→await→finalize lifecycle across all ranks over RPC
  • channels/: Input, Output, Message, MessageTypeRegistry — host-side SPSC channel primitives

Examples (examples/)

  • broadcastDeployment, channelController (telephone game), service (hello-world service loop), engine (bare engine bring-up)

Docs (docs/)

  • architecture.md: instance roles (deployer / coordinator / replica / request manager), module lifecycle, channel model, deferred scope
  • simpler-integration.md: how replica ranks launch Simpler (deviceId = instanceId - numPartitions), processFc contract, path to in-device tensor channels
  • model-support-interface.md: what Model Support provides vs what the platform owns, future RuntimePlan contract

What is explicitly deferred

  • channelDispatcher, taskScheduler, executor roles (coordinator, replica), heartbeat
  • In-device tensor channels (the TP/PP hot path without host staging) — tracked in [Feature] Platform Management Design #32
  • RuntimePlan versioned snapshot and watch/subscribe API
  • Dynamic scaling, drain, fault recovery, Python bindings

Relation to open issues

Closes part of #32. The executor roles and in-device tensor channels are the remaining items needed before the platform can fully replace manual MPI bootstrapping in model serving workloads.

@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b307f9b9-b982-4169-ad05-4ea9c06404a5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main platform control-plane and configuration additions.
Description check ✅ Passed The description matches the changeset and accurately summarizes the new configuration, modules, examples, and docs.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a modular platform architecture with reusable components, including a base module lifecycle, configuration parsing and validation, an RPC-based deployment broadcaster, a channel controller for desired-state reconciliation, and a TaskR service adapter. Several critical issues were identified in the review: a data race in the channel controller's status checks due to missing mutex locks, a format string typo in the deployment validation logic, uninitialized member variables in multiple configuration classes during deserialization, and a bug in the configuration helper that assigns the coordinator's ID to all replicas instead of unique instance IDs.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread platform/include/modules/channelController/module.hpp Outdated
Comment thread platform/include/modules/configuration/deployment.hpp Outdated
Comment thread platform/include/modules/configuration/partition.hpp
Comment thread platform/include/modules/configuration/replica.hpp
Comment thread platform/include/modules/configuration/requestManager.hpp
Comment thread platform/examples/include/deployment/helpers.hpp
lterrac added a commit that referenced this pull request Jul 2, 2026
- Add mutex locks to hasProducer/hasConsumer to prevent data race
  with the reconcile service thread modifying _actual* maps
- Fix format string typo '%''' → '%s' in deployment verify()

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
lterrac added a commit that referenced this pull request Jul 2, 2026
- Add mutex locks to hasProducer/hasConsumer to prevent data race
  with the reconcile service thread modifying _actual* maps
- Fix format string typo '%''' → '%s' in deployment verify()

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lterrac lterrac force-pushed the feat/platform-config branch from 18234a6 to 21ce8b4 Compare July 2, 2026 10:42

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🧹 Nitpick comments (11)
platform/examples/include/channels/helpers.hpp (2)

64-78: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

No null-check after .lock() on weak_ptr.

addDesiredConsumer/addDesiredProducer return a weak_ptr; calling .lock() can return nullptr if the underlying object already expired. Both createDesiredInput and createDesiredOutput return this result unchecked, so downstream callers (e.g., waitUntilReady) could dereference a null pointer.

🛡️ Proposed fix
   auto input = channelController->addDesiredConsumer(inputInfo.sourceInstanceId, inputInfo.channelId, *inputInfo.edge, keyBuilder).lock();
+  if (input == nullptr) HICR_THROW_LOGIC("Failed to acquire desired input channel.");
   return input;
🤖 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 `@platform/examples/include/channels/helpers.hpp` around lines 64 - 78, The
helper functions createDesiredInput and createDesiredOutput return the result of
locking a weak_ptr from addDesiredConsumer/addDesiredProducer without verifying
it succeeded. Update both functions to check the .lock() result from
channelController before returning, and handle an expired weak_ptr explicitly so
callers like waitUntilReady never receive a null Input/Output pointer
unexpectedly. Use the existing createDesiredInput, createDesiredOutput, and
channelController::Module calls to locate the fix.

116-119: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

waitUntilReady has no timeout.

Busy-polling with isReady() indefinitely can hang forever if the channel never becomes ready (e.g., peer failure). Consider adding an optional timeout/max-attempts parameter.

🤖 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 `@platform/examples/include/channels/helpers.hpp` around lines 116 - 119, The
waitUntilReady helper currently busy-waits on Base::isReady() forever, so add an
optional timeout or max-attempts parameter to the waitUntilReady function and
stop waiting once it is exceeded. Update the loop to track elapsed time or
attempts, preserve the existing intervalMs behavior, and make sure callers can
still use the current signature by defaulting the new parameter.
platform/examples/include/deployment/helpers.hpp (1)

202-219: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Unchecked argv[1] access and fallthrough after abort().

argv[1] is dereferenced without validating argc >= 2. Also, after calling instanceManager->abort(-1) on insufficient instances, execution falls through to the rest of the function (no return); if abort() does not unconditionally terminate the process, the subsequent partition-instance assignment loop will run with insufficient instances.

🛡️ Proposed fix
   if (instanceManager->getInstances().size() < instancesRequired)
   {
     fprintf(stderr, "Error: %zu instances provided, but %lu are required\n", instanceManager->getInstances().size(), instancesRequired);
     instanceManager->abort(-1);
+    return;
   }
🤖 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 `@platform/examples/include/deployment/helpers.hpp` around lines 202 - 219,
Validate argc before using argv[1] in the helper that builds deployment config,
and bail out with an error if no config path was provided. Also make the
insufficient-instances path in deployment::deserialize/setup stop execution
explicitly after instanceManager->abort(-1) so the partition assignment logic
cannot continue; use the existing servingConfigFilePath, instanceManager, and
instancesRequired checks to place the guard in the right spot.
platform/examples/include/runtime/helpers.hpp (1)

52-58: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

hwloc_topology_init result is never released.

runtime.hwlocTopologyObject is initialized but there's no corresponding hwloc_topology_destroy anywhere (no destructor on Runtime). This leaks the hwloc topology object for the process lifetime.

🤖 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 `@platform/examples/include/runtime/helpers.hpp` around lines 52 - 58, The
hwloc topology initialized in the runtime setup is never released, causing a
leak tied to Runtime lifetime. Update the Runtime ownership path so the hwloc
topology created by hwloc_topology_init is destroyed later, ideally by adding
cleanup in Runtime’s destructor (or equivalent RAII wrapper) and ensuring any
code using TopologyManager still only accesses it while valid. Use the existing
runtime.hwlocTopologyObject and hwloc_topology_init symbols to locate the
setup/teardown pair.
platform/include/modules/configuration/requestManager.hpp (1)

15-15: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Pass JSON by const reference to avoid an unnecessary copy.

♻️ Proposed fix
-  RequestManager(const nlohmann::json js) { deserialize(js); };
+  RequestManager(const nlohmann::json &js) { deserialize(js); };
🤖 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 `@platform/include/modules/configuration/requestManager.hpp` at line 15, The
RequestManager constructor currently takes nlohmann::json by value, causing an
unnecessary copy; update the RequestManager constructor signature to accept the
JSON payload as a const reference and keep deserialization behavior unchanged.
Use the RequestManager constructor and deserialize method as the key symbols
when updating the API.
platform/include/modules/configuration/partition.hpp (1)

20-20: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Pass JSON by const reference to avoid an unnecessary copy.

Partition(const nlohmann::json js) copies the whole nlohmann::json object; Task/Edge/Deployment already take it by const &.

♻️ Proposed fix
-  Partition(const nlohmann::json js) { deserialize(js); };
+  Partition(const nlohmann::json &js) { deserialize(js); };
🤖 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 `@platform/include/modules/configuration/partition.hpp` at line 20, The
Partition constructor currently takes nlohmann::json by value, which creates an
unnecessary copy; update Partition::Partition to accept the json as a const
reference to match Task/Edge/Deployment and avoid copying, while keeping the
deserialize(js) call unchanged.
platform/include/modules/configuration/replica.hpp (1)

17-17: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Pass JSON by const reference to avoid an unnecessary copy.

♻️ Proposed fix
-  Replica(const nlohmann::json js) { deserialize(js); };
+  Replica(const nlohmann::json &js) { deserialize(js); };
🤖 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 `@platform/include/modules/configuration/replica.hpp` at line 17, The Replica
constructor is taking a nlohmann::json by value, causing an unnecessary copy;
update the Replica constructor to accept the JSON input by const reference and
keep using deserialize(js) inside the constructor. Use the Replica class
constructor signature as the target for the change so the deserialization path
remains the same without copying the input.
platform/include/modules/configuration/edge.hpp (1)

16-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move the macro definition out of the class body.

__SERVING_PARTITION_DEFAULT_BUFFER_CAPACITY is defined inside class Edge, which is misleading — preprocessor macros aren't scoped to the class and leak into the global namespace for the rest of the translation unit regardless of where they're textually placed.

♻️ Proposed fix
+#define __SERVING_PARTITION_DEFAULT_BUFFER_CAPACITY 1
+
 namespace serving::configuration
 {
 
 class Edge
 {
   public:
 
-#define __SERVING_PARTITION_DEFAULT_BUFFER_CAPACITY 1
-
   typedef uint64_t edgeIndex_t;
🤖 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 `@platform/include/modules/configuration/edge.hpp` around lines 16 - 19, The
macro __SERVING_PARTITION_DEFAULT_BUFFER_CAPACITY is currently placed inside
class Edge, but macros are not class-scoped and still leak globally. Move that
`#define` out of the Edge class body in edge.hpp to a top-level location with the
other preprocessor definitions, and keep the class body limited to actual
members and methods.
platform/include/modules/service/module.hpp (1)

21-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider moving the shared_ptr and marking the constructor explicit.

taskr is taken by value but copied into _taskr instead of moved, incurring an unnecessary atomic refcount bump. Also, a single-argument constructor is a good candidate for explicit to avoid accidental implicit conversions.

♻️ Proposed tweak
-  Module(std::shared_ptr<taskr::Runtime> taskr)
+  explicit Module(std::shared_ptr<taskr::Runtime> taskr)
     : serving::modules::Module(),
-      _taskr(taskr)
+      _taskr(std::move(taskr))
   {}
🤖 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 `@platform/include/modules/service/module.hpp` around lines 21 - 24, Update the
`Module` constructor in `serving::modules::Module` to take the
`std::shared_ptr<taskr::Runtime>` by value only if it is intentionally copied,
but preferably change it to accept a movable/shared ownership parameter and move
it into `_taskr` instead of copying; also mark this single-argument constructor
`explicit` to prevent unintended implicit conversions.
platform/include/modules/channelController/module.hpp (1)

214-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Stale/misleading map-key comments.

Comments state // key: target instance / // key: source instance, but the maps are keyed by edgeName_t (edge name, see edge.getName() at lines 60/72), not instance id.

✏️ Suggested fix
-  // Desired state
-  std::unordered_map<edgeName_t, std::shared_ptr<output_t>> _desiredProducers; // key: target instance
-  std::unordered_map<edgeName_t, std::shared_ptr<input_t>>  _desiredConsumers; // key: source instance
-
-  // Actual created state
-  std::unordered_map<edgeName_t, std::shared_ptr<output_t>> _actualProducers; // key: target instance
-  std::unordered_map<edgeName_t, std::shared_ptr<input_t>>  _actualConsumers; // key: source instance
+  // Desired state
+  std::unordered_map<edgeName_t, std::shared_ptr<output_t>> _desiredProducers; // key: edge name
+  std::unordered_map<edgeName_t, std::shared_ptr<input_t>>  _desiredConsumers; // key: edge name
+
+  // Actual created state
+  std::unordered_map<edgeName_t, std::shared_ptr<output_t>> _actualProducers; // key: edge name
+  std::unordered_map<edgeName_t, std::shared_ptr<input_t>>  _actualConsumers; // key: edge name
🤖 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 `@platform/include/modules/channelController/module.hpp` around lines 214 -
220, The map-key comments on _desiredProducers, _desiredConsumers,
_actualProducers, and _actualConsumers are misleading because these
unordered_maps are keyed by edgeName_t rather than instance identity. Update the
inline comments in module.hpp to describe the actual key consistently with the
edge name used by ChannelController and edge.getName(), so the declarations
match the stored lookup key.
platform/include/modules/broadcastDeployment/module.hpp (1)

24-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate RPC-target registration across constructors.

Both constructors repeat identical registration logic. A delegating constructor removes the duplication and the risk of the two diverging over time.

♻️ Proposed refactor using a delegating constructor
   Module(std::shared_ptr<HiCR::InstanceManager>     instanceManager,
          std::shared_ptr<HiCR::ComputeManager>      computeManager,
          std::shared_ptr<HiCR::frontend::RPCEngine> rpcEngine,
          const HiCR::Instance::instanceId_t         deployerInstanceId,
          const HiCR::Instance::instanceId_t         instanceId,
          const configuration::Deployment           &deployment)
-    : modules::Module(),
-      _instanceManager(instanceManager),
-      _computeManager(computeManager),
-      _rpcEngine(rpcEngine),
-      _deployerInstanceId(deployerInstanceId),
-      _instanceId(instanceId),
-      _deployment(deployment)
-  {
-    _rpcEngine->addRPCTarget(__SERVING_REQUEST_DEPLOYMENT_CONFIGURATION_RPC_NAME, _computeManager->createExecutionUnit([this](void *) { sendDeploymentConfiguration(); }));
-  }
+    : Module(instanceManager, computeManager, rpcEngine, deployerInstanceId, instanceId)
+  {
+    _deployment = deployment;
+  }

   Module(std::shared_ptr<HiCR::InstanceManager>     instanceManager,
          std::shared_ptr<HiCR::ComputeManager>      computeManager,
          std::shared_ptr<HiCR::frontend::RPCEngine> rpcEngine,
          const HiCR::Instance::instanceId_t         deployerInstanceId,
          const HiCR::Instance::instanceId_t         instanceId)
     : modules::Module(),
       _instanceManager(instanceManager),
       _computeManager(computeManager),
       _rpcEngine(rpcEngine),
       _deployerInstanceId(deployerInstanceId),
       _instanceId(instanceId)
   {
     _rpcEngine->addRPCTarget(__SERVING_REQUEST_DEPLOYMENT_CONFIGURATION_RPC_NAME, _computeManager->createExecutionUnit([this](void *) { sendDeploymentConfiguration(); }));
   }
🤖 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 `@platform/include/modules/broadcastDeployment/module.hpp` around lines 24 -
54, Both Module constructors duplicate the same addRPCTarget registration for
__SERVING_REQUEST_DEPLOYMENT_CONFIGURATION_RPC_NAME, which can drift over time;
refactor the overloads in Module to use a delegating constructor or a shared
initialization helper so the RPC-target setup lives in one place and both
constructors reuse it consistently.
🤖 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 `@platform/examples/include/runtime/helpers.hpp`:
- Around line 59-78: The helper in `helpers.hpp` is ignoring the
`computeResourceCount` argument and can walk past the available compute
resources. Update the `computeResourcesIt` population loop to stop at
`device->getComputeResourceList().end()` (or otherwise validate availability)
before dereferencing, and replace the hardcoded `runtime.computeResources.size()
!= 2` check with a comparison against `computeResourceCount` (or another derived
expected count) so `runtime.rpcEngine` and `runtime.taskr` work correctly for
custom resource counts.

In `@platform/examples/modules/channelController/channelController.cpp`:
- Around line 36-43: The argc validation in channelController.cpp is missing a
hard stop after runtime.instanceManager->abort(-1), so execution can still reach
readAndParseConfiguration and access argv[1] when it is invalid. Update the main
flow around the argc check to return immediately after aborting, or otherwise
ensure the function does not continue past the error path. Keep the fix
localized to the argc guard and the readAndParseConfiguration call site so argv
is only used when argc is valid.

In `@platform/examples/modules/channelController/telephoneGame.hpp`:
- Around line 35-46: The non-root path in TelephoneGame is consuming a message
with inputChannel.getMessage() but never removing it from the channel buffer.
Update the else branch in telephoneGame.hpp so that after reading and logging
the message, it also calls inputChannel.popMessage() like the root branch does,
keeping the channel drained before teardown/removal. Use the existing
inputChannel.getMessage() and inputChannel.popMessage() flow in TelephoneGame as
the place to fix this.

In `@platform/examples/modules/service/service.cpp`:
- Around line 34-46: The service setup in the topology/device selection block
assumes at least one memory space and two compute resources, which can
dereference empty or exhausted iterators. Update the logic around
getMemorySpaceList(), getComputeResourceList(), and the loop that fills
computeResources to validate availability before dereferencing, and handle the
insufficient-resources case gracefully by either selecting fewer resources or
failing early with a clear message.

In `@platform/include/modules/configuration/deployment.hpp`:
- Around line 30-43: The heartbeat_t members are left uninitialized on default
construction, so Deployment::serialize() can read garbage before deserialize()
runs. Add default member initializers to heartbeat_t for enabled, visible,
interval, and tolerance, matching the initialization style used by
controlBuffer_t, and keep the fix localized to the heartbeat_t struct in
deployment.hpp.

In `@platform/include/modules/configuration/partition.hpp`:
- Around line 62-63: The `_coordinatorInstanceId` member in `Partition` can
remain uninitialized when `deserialize()` skips the optional "Coordinator
Instance Id" field, so add a safe default member initializer or equivalent
construction-time initialization and ensure `getCoordinatorInstanceId()`,
`serialize()`, and `setCoordinatorInstanceId()` all work from a known state.
Update the `Partition` class definition and the `deserialize` logic so the
optional JSON field only overrides a valid default instead of leaving the member
undefined.

In `@platform/include/modules/configuration/replica.hpp`:
- Around line 36-38: The Replica configuration deserialization leaves
_instanceId uninitialized when "Instance Id" is missing, so initialize it to a
safe default at declaration or in the Replica constructor and keep deserialize()
only overriding it when the JSON key exists. Update the Replica class member and
any related serialize()/getInstanceId() paths to assume a valid default state
rather than relying on conditional assignment.

In `@platform/include/modules/configuration/requestManager.hpp`:
- Around line 41-45: The deserialize method in RequestManager leaves _instanceId
uninitialized when the JSON omits "Instance Id", so initialize it to a safe
default in the class definition or at the start of deserialize and keep the
conditional assignment for present values. Use the same approach as
Partition::_coordinatorInstanceId and Replica::_instanceId so
RequestManager::_instanceId is always valid before any read.

---

Nitpick comments:
In `@platform/examples/include/channels/helpers.hpp`:
- Around line 64-78: The helper functions createDesiredInput and
createDesiredOutput return the result of locking a weak_ptr from
addDesiredConsumer/addDesiredProducer without verifying it succeeded. Update
both functions to check the .lock() result from channelController before
returning, and handle an expired weak_ptr explicitly so callers like
waitUntilReady never receive a null Input/Output pointer unexpectedly. Use the
existing createDesiredInput, createDesiredOutput, and channelController::Module
calls to locate the fix.
- Around line 116-119: The waitUntilReady helper currently busy-waits on
Base::isReady() forever, so add an optional timeout or max-attempts parameter to
the waitUntilReady function and stop waiting once it is exceeded. Update the
loop to track elapsed time or attempts, preserve the existing intervalMs
behavior, and make sure callers can still use the current signature by
defaulting the new parameter.

In `@platform/examples/include/deployment/helpers.hpp`:
- Around line 202-219: Validate argc before using argv[1] in the helper that
builds deployment config, and bail out with an error if no config path was
provided. Also make the insufficient-instances path in
deployment::deserialize/setup stop execution explicitly after
instanceManager->abort(-1) so the partition assignment logic cannot continue;
use the existing servingConfigFilePath, instanceManager, and instancesRequired
checks to place the guard in the right spot.

In `@platform/examples/include/runtime/helpers.hpp`:
- Around line 52-58: The hwloc topology initialized in the runtime setup is
never released, causing a leak tied to Runtime lifetime. Update the Runtime
ownership path so the hwloc topology created by hwloc_topology_init is destroyed
later, ideally by adding cleanup in Runtime’s destructor (or equivalent RAII
wrapper) and ensuring any code using TopologyManager still only accesses it
while valid. Use the existing runtime.hwlocTopologyObject and
hwloc_topology_init symbols to locate the setup/teardown pair.

In `@platform/include/modules/broadcastDeployment/module.hpp`:
- Around line 24-54: Both Module constructors duplicate the same addRPCTarget
registration for __SERVING_REQUEST_DEPLOYMENT_CONFIGURATION_RPC_NAME, which can
drift over time; refactor the overloads in Module to use a delegating
constructor or a shared initialization helper so the RPC-target setup lives in
one place and both constructors reuse it consistently.

In `@platform/include/modules/channelController/module.hpp`:
- Around line 214-220: The map-key comments on _desiredProducers,
_desiredConsumers, _actualProducers, and _actualConsumers are misleading because
these unordered_maps are keyed by edgeName_t rather than instance identity.
Update the inline comments in module.hpp to describe the actual key consistently
with the edge name used by ChannelController and edge.getName(), so the
declarations match the stored lookup key.

In `@platform/include/modules/configuration/edge.hpp`:
- Around line 16-19: The macro __SERVING_PARTITION_DEFAULT_BUFFER_CAPACITY is
currently placed inside class Edge, but macros are not class-scoped and still
leak globally. Move that `#define` out of the Edge class body in edge.hpp to a
top-level location with the other preprocessor definitions, and keep the class
body limited to actual members and methods.

In `@platform/include/modules/configuration/partition.hpp`:
- Line 20: The Partition constructor currently takes nlohmann::json by value,
which creates an unnecessary copy; update Partition::Partition to accept the
json as a const reference to match Task/Edge/Deployment and avoid copying, while
keeping the deserialize(js) call unchanged.

In `@platform/include/modules/configuration/replica.hpp`:
- Line 17: The Replica constructor is taking a nlohmann::json by value, causing
an unnecessary copy; update the Replica constructor to accept the JSON input by
const reference and keep using deserialize(js) inside the constructor. Use the
Replica class constructor signature as the target for the change so the
deserialization path remains the same without copying the input.

In `@platform/include/modules/configuration/requestManager.hpp`:
- Line 15: The RequestManager constructor currently takes nlohmann::json by
value, causing an unnecessary copy; update the RequestManager constructor
signature to accept the JSON payload as a const reference and keep
deserialization behavior unchanged. Use the RequestManager constructor and
deserialize method as the key symbols when updating the API.

In `@platform/include/modules/service/module.hpp`:
- Around line 21-24: Update the `Module` constructor in
`serving::modules::Module` to take the `std::shared_ptr<taskr::Runtime>` by
value only if it is intentionally copied, but preferably change it to accept a
movable/shared ownership parameter and move it into `_taskr` instead of copying;
also mark this single-argument constructor `explicit` to prevent unintended
implicit conversions.
🪄 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: 6d4cadec-c166-4cfa-bc9f-4f54f5d05200

📥 Commits

Reviewing files that changed from the base of the PR and between e35fe71 and 21ce8b4.

📒 Files selected for processing (28)
  • .agents/skills/README.md
  • platform/docs/modules/README.md
  • platform/docs/modules/broadcast-deployment.md
  • platform/docs/modules/channel-controller.md
  • platform/docs/modules/configuration.md
  • platform/docs/modules/service.md
  • platform/examples/include/channels/helpers.hpp
  • platform/examples/include/deployment/helpers.hpp
  • platform/examples/include/runtime/helpers.hpp
  • platform/examples/meson.build
  • platform/examples/modules/broadcastDeployment/broadcastDeployment.cpp
  • platform/examples/modules/broadcastDeployment/meson.build
  • platform/examples/modules/broadcastDeployment/policy.json
  • platform/examples/modules/channelController/channelController.cpp
  • platform/examples/modules/channelController/meson.build
  • platform/examples/modules/channelController/policy.json
  • platform/examples/modules/channelController/telephoneGame.hpp
  • platform/examples/modules/service/meson.build
  • platform/examples/modules/service/service.cpp
  • platform/include/modules/broadcastDeployment/module.hpp
  • platform/include/modules/channelController/module.hpp
  • platform/include/modules/configuration/deployment.hpp
  • platform/include/modules/configuration/edge.hpp
  • platform/include/modules/configuration/partition.hpp
  • platform/include/modules/configuration/replica.hpp
  • platform/include/modules/configuration/requestManager.hpp
  • platform/include/modules/configuration/task.hpp
  • platform/include/modules/service/module.hpp

Comment thread platform/examples/include/runtime/helpers.hpp
Comment thread platform/examples/modules/channelController/channelController.cpp
Comment thread platform/examples/modules/channelController/telephoneGame.hpp
Comment thread platform/examples/modules/service/service.cpp Outdated
Comment thread platform/include/modules/configuration/deployment.hpp
Comment thread platform/include/modules/configuration/partition.hpp
Comment thread platform/include/modules/configuration/replica.hpp
Comment thread platform/include/modules/configuration/requestManager.hpp
lterrac added a commit that referenced this pull request Jul 2, 2026
- telephoneGame: call popMessage() after non-root branch consumes message
- channelController: return -1 after abort() so argv[1] is never read on bad argc
- service: guard memSpaces and computeResources iterators before dereferencing
- partition: default _coordinatorInstanceId to 0 (safe for single-node deployments)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lterrac lterrac marked this pull request as ready for review July 2, 2026 12:07
lterrac added a commit that referenced this pull request Jul 7, 2026
- Add mutex locks to hasProducer/hasConsumer to prevent data race
  with the reconcile service thread modifying _actual* maps
- Fix format string typo '%''' → '%s' in deployment verify()

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
lterrac added a commit that referenced this pull request Jul 7, 2026
- telephoneGame: call popMessage() after non-root branch consumes message
- channelController: return -1 after abort() so argv[1] is never read on bad argc
- service: guard memSpaces and computeResources iterators before dereferencing
- partition: default _coordinatorInstanceId to 0 (safe for single-node deployments)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lterrac lterrac force-pushed the feat/platform-config branch from 2c751d2 to 255170e Compare July 7, 2026 12:47
lterrac and others added 5 commits July 13, 2026 10:12
…Deployment, and service modules

Ports the configuration layer (Deployment, Partition, Replica, Task, Edge,
RequestManager) and three modules from the upstream reference implementation,
renaming the namespace to serving throughout. Wires the new module examples
into the meson build and verifies the full build is clean.

- configuration/: JSON-serializable deployment graph types; Deployment.verify()
  checks edge connectivity, producer/consumer symmetry, and request-manager
  boundary constraints
- channelController: reconciliation-loop module that diffs desired vs actual
  Input/Output channels and drives exchange/fence/initialize in one pass
- broadcastDeployment: RPC-based module that distributes the serialized
  Deployment from the deployer instance to all workers at startup
- service: thin Module wrapper around taskr::Runtime for cooperative-polling
  services; addService() registers external taskr::Service pointers
- examples/include/: deployment helpers (readAndParseConfiguration,
  buildLocalChannelsFromDeployment, assignEdgeManagers) and channel helpers
  (createDesiredSingleLocalChannels, waitUntilReady, edge name builders)
- examples/modules/: broadcastDeployment, channelController, and service
  example programs with policy.json fixtures and meson test targets

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add mutex locks to hasProducer/hasConsumer to prevent data race
  with the reconcile service thread modifying _actual* maps
- Fix format string typo '%''' → '%s' in deployment verify()

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The coordinator and its first replica share the same instance, so
additional replicas (i > 0) each need their own distinct instance ID.
The required instance count is adjusted accordingly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- telephoneGame: call popMessage() after non-root branch consumes message
- channelController: return -1 after abort() so argv[1] is never read on bad argc
- service: guard memSpaces and computeResources iterators before dereferencing
- partition: default _coordinatorInstanceId to 0 (safe for single-node deployments)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ort interface docs

- architecture.md: instance roles, module lifecycle, channel model, deferred scope
- simpler-integration.md: replica device assignment, RuntimeBinaries, processFc contract,
  hot-path staging and future in-device tensor channels
- model-support-interface.md: what Model Support provides vs owns, RuntimePlan future contract
- README.md: updated index linking all new and existing docs

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant