Record conv1d/conv3d through the recording decorator (#532)#533
Merged
michalharakal merged 1 commit intodevelopfrom Apr 19, 2026
Merged
Record conv1d/conv3d through the recording decorator (#532)#533michalharakal merged 1 commit intodevelopfrom
michalharakal merged 1 commit intodevelopfrom
Conversation
RecordingTensorOpsDecorator.conv1d and .conv3d overrode the base TensorOps but only called base.conv*(...) — they never invoked record(...), so every conv1d/conv3d call routed through Execution.recordingOps(...) was silently dropped from the tape. conv2d always recorded correctly. Supporting Conv1dOperation and Conv3dOperation classes also didn't exist. - Add Conv1dOperation and Conv3dOperation mirroring Conv2dOperation. - Wire conv1d and conv3d in the decorator to record the operation along with stride / padding / dilation / groups, same as conv2d. - Extend the stable-input-name helper so the new conv ops get input / weight / bias naming. - Add SimpleExecutionTapeTest cases that route a conv1d and a conv3d through the decorator and assert the tape captured the ops, the stable names, the static input / output shapes, and the parameters. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
📖 Documentation Preview The documentation has been built successfully for this PR. Generated Files:
Artifacts:
This comment will be updated automatically when the PR is updated. |
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
1 participant
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.
Fixes #532.
Summary
RecordingTensorOpsDecorator.conv1d/.conv3dnow callrecord(...)withConv1dOperation/Conv3dOperationand the stride / padding / dilation / groups parameters, matching whatconv2dhas always done. Previously both overrode the base interface but only forwarded tobase.conv*(...)and dropped the trace entirely.Conv1dOperationandConv3dOperationclasses inskainet-lang-core(they didn't exist — onlyConv2dOperationdid). They mirrorConv2dOperation's behaviour.stableInputNamehelper inRecordingExecution.ktso the new conv ops get theinput/weight/biasnaming already used byconv2d.SimpleExecutionTapeTest.records_conv1d_through_recording_decoratorandrecords_conv3d_through_recording_decoratorthat route calls throughExecution.recordingOpsand assert the tape captured the ops with correct stable names, static input / output shapes, and parameters.Context
Discovered while investigating #530 (the tape's other recording path, via
DefaultExecutionTape.recordTrace, was also losing static conv1d shapes). #530 already unblocks the Whisper → IREE / Vulkan pipeline (that path uses the KSP-generated tracer). This PR closes the symmetric gap on theExecution.recordingOps(...)public API so both recording paths stay consistent.Test plan
./gradlew :skainet-compile:skainet-compile-core:jvmTest --tests 'sk.ainet.tape.SimpleExecutionTapeTest.*'passes locally.apiCheckoutput forskainet-lang-coreshows only the three newConv{1,3}dOperationentries compared to develop (develop's apiCheck has pre-existing unrelated drift, not touched here).🤖 Generated with Claude Code