Skip to content

Creating Runtime Module for IR to IO , adding libsvm support(wip)#5

Open
pantShrey wants to merge 7 commits into
armanbilge:mainfrom
pantShrey:libsvm_integration
Open

Creating Runtime Module for IR to IO , adding libsvm support(wip)#5
pantShrey wants to merge 7 commits into
armanbilge:mainfrom
pantShrey:libsvm_integration

Conversation

@pantShrey

Copy link
Copy Markdown
Collaborator

The outputs are inconsistent
Sometimes i get proper response like

Loading model from: static_svm.onnx
Model loaded. Translating to ModelIR...
Translation successful.

--- Running Interpreter ---
Inference complete!

--- Inference Outputs ---
Output tensor 'label': Array(1)
Output tensor 'probabilities': Array(1.1038835E-38, 1.4E-45)
Process 32574 exited with status = 0 (0x00000000) 

But majorly I am getting error

Loading model from: static_svm.onnx
Model loaded. Translating to ModelIR...
Translation successful.

--- Running Interpreter ---
Process 32570 stopped
* thread #5, stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x0000000100b5d38c libsvm.4.dylib`svm_predict_values + 160
libsvm.4.dylib`svm_predict_values:
->  0x100b5d38c <+160>: ldr    x1, [x26], #0x8
    0x100b5d390 <+164>: mov    x0, x22
    0x100b5d394 <+168>: mov    x2, x19
    0x100b5d398 <+172>: bl     0x100b58cd4    ; Kernel::k_function(svm_node const*, svm_node const*, svm_parameter const&)

Right(List.empty[Allocation])
}
}
} yield newAllocs.flatten

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I wonder if instead of flatten can use flatTraverse.

Comment on lines +108 to +111
case "SVMClassifier" =>
for {
_ <- checkArity(node, 1, 2) // Ensure SVMClassifier has 2 outputs
scoresOutputName = node.output(1)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Some of the repeated logic is unfortunate here. Not for this PR, but in a follow-up PR I wonder if we can combine both these phases in single-pass. So, the translator would generate both IR operations and manual allocations simultaneously as it traverses the ONNX operations.

Comment thread build.sbt Outdated
lazy val runtime = project
.enablePlugins(ScalaNativePlugin, ScalaNativeBrewedConfigPlugin)
.dependsOn(ir)
.dependsOn(ir, onnx)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is this just for testing? I think the runtime shouldn't directly depend on ONNX.

Suggested change
.dependsOn(ir, onnx)
.dependsOn(ir, onnx % Test)

Comment on lines +262 to +263
/** Copies input Scala Arrays into their corresponding pre-allocated C memory. */
private def copyInputsToMemory(

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Instead of copying the values, we can use Scala Native array.unsafeAt(0) operation to get a Ptr. That is the magic of Scala Native, we can directly do interop without needing to copy data between Scala and native data structures :)

Comment on lines +288 to +289
/** Copies final results from C memory back into new Scala Arrays. */
private def copyOutputsFromMemory(model: ModelIR, memory: MemoryMap): IO[Map[String, Array[_]]] =

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Same thing here, instead of copying the memory back, we can directly write into the result array.

Comment on lines +77 to +80
case other =>
IO.raiseError(
new NotImplementedError(s"Operation not implemented: ${other.getClass.getSimpleName}"),
)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Similar idea here, we can actually raise this error outside of the IO so that it surfaces earlier.

Comment on lines +85 to +87
private def handleElementWise(op: Operation, memory: MemoryMap, model: ModelIR)(
f: (Float, Float) => Float,
): IO[Unit] = IO {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Unfortunate the f: (Float, Float) => Float will be allocated as a lambda and use dynamic dispatch. The Scala Native Interflow optimizer may be able to specialize this and avoid the lambda allocation and dispatch, but unfortunately it would be better to write these as separate methods with the operation hard-coded, for the guaranteed best performance.

// This entire block is now a single IO action that performs the C interop.
predictionResult <- IO {
// 1. Fill the svm_node struct with input data from the input tensor.
for (i <- 0 until numFeatures) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We should use a while loop with manually-incremented counter instead of a for (which desugars to a Range allocation, foreach, and lambda with dynamic dispatch).

// 2. Prepare for and call the prediction function.
val nrClass = op.classLabels.size
val decValuesCount = nrClass * (nrClass - 1) / 2
val decisionValuesPtr = stackalloc[CDouble](decValuesCount.toUInt)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I see that you are returning decisionValuesPtr at the end of the IO, but this is not safe to do with stackallocated memory. The stack is popped at the end of the IO, so that memory is no longer safe to access.

Comment on lines +141 to +142
// Using a for-comprehension here breaks the logic into clean, sequential IO steps.
for {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Actually, I think it would be okay to write the logic within a single IO.

* @return
* An IO containing a map of output tensor names to their resulting Scala arrays.
*/
def execute(model: ModelIR, inputs: Map[String, Array[_]]): IO[Map[String, Array[_]]] = {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
def execute(model: ModelIR, inputs: Map[String, Array[_]]): IO[Map[String, Array[_]]] = {
def execute(model: ModelIR, inputs: Map[String, Array[_]]): Resource[IO, IO[Map[String, Array[_]]]] = {

Comment on lines +54 to +55
memoryResource(model, inputs, outputArrays)
.use { memoryMap =>

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
memoryResource(model, inputs, outputArrays)
.use { memoryMap =>
memoryResource(model, inputs, outputArrays)
.flatMap { memoryMap =>

op: Operation.SVMClassifier,
memory: MemoryMap,
model: ModelIR,
): IO[Unit] = {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
): IO[Unit] = {
): Resource[IO, IO[Unit]] = {

svmInputNode <- malloc[svm_node](sizeof[svm_node] * (numFeatures + 1).toUSize)
} yield (modelPtr, svmInputNode)

svmResources.use { case (modelPtr, svmInputNode) =>

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
svmResources.use { case (modelPtr, svmInputNode) =>
svmResources.map { case (modelPtr, svmInputNode) =>

@armanbilge armanbilge marked this pull request as ready for review August 4, 2025 19:58
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.

2 participants