Support for MaxPool1D and RQSConv1D for PULPOpen #146
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds 1D MaxPool and RQSConv1D support for both Generic and PULPOpen platforms, including kernel implementations, templates, bindings, tiling constraints, and platform mappings. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@Deeploy/Targets/PULPOpen/Templates/MaxPoolTemplate.py`:
- Around line 34-46: The template PULPMaxPool1D_8_Template/PULPMaxPoolTemplate
currently passes ONNX 1D pads into the HEIGHT pad slots of pulp_nn_maxpool (so
padding_top/padding_bottom get ${padding_y}/${padding_y_right}); swap the pad
ordering so height pads are zeros and width pads receive the ONNX values: set
padding_top=0, padding_bottom=0 and padding_left=${padding_y},
padding_right=${padding_y_right} in the pulp_nn_maxpool${signatureString} call
(i.e. move the two zeros before the two ${padding_y} args).
In `@Deeploy/Targets/PULPOpen/TileConstraints/ConvTileConstraint.py`:
- Around line 840-844: The effectiveInputLengthVar calculation is wrong: it
subtracts (inputLengthVar - 1) for the tiled case but should subtract the kernel
span (weightLengthVar - 1); update the expression in ConvTileConstraint (the
effectiveInputLengthVar assignment) to replace the final term ((inputLengthVar -
1) * (inputLengthVar != inputBuffer.shape[1])) with ((weightLengthVar - 1) *
(inputLengthVar != inputBuffer.shape[1])) so the receptive field overlap uses
weightLengthVar consistently with the 2D implementation.
- Around line 871-873: The effectiveInputLength calculation in
addPolicyConstraint uses (inputLengthVar - 1) where it should subtract the
kernel length minus one (same bug as in addGeometricalConstraint); update the
ternary expression so the last term uses the kernel size (e.g., kernelLength or
kernelLengthVar) minus one instead of inputLengthVar - 1, i.e., change "-
(inputLengthVar - 1) * (inputLengthVar != inputBuffer.shape[1])" to "-
(kernelLength - 1) * (inputLengthVar != inputBuffer.shape[1])" in the
effectiveInputLength assignment inside addPolicyConstraint.
In `@TargetLibraries/Generic/src/MaxPool_s8.c`:
- Around line 52-69: The MaxPool1d_s8_s8 function currently computes L_out = (L
- K) / S + 1 which can underflow when K > L and divide-by-zero when S == 0;
before calculating L_out, add guards: if S == 0 return (or otherwise abort) and
if K > L set L_out = 0 (or return) to avoid the underflow/OOB behavior; update
the function entry (MaxPool1d_s8_s8) to validate S and the relationship of K and
L and bail out or produce zero-length output before any loops that use L_out.
🧹 Nitpick comments (5)
TargetLibraries/Generic/inc/kernel/MaxPool.h (1)
42-48: Consider aligning parameter naming between int8 and fp32 variants.The
MaxPool1d_s8_s8function usesLfor the spatial dimension whileMaxPool1d_fp32_fp32usesW. For consistency across the API, consider using the same parameter name (e.g.,Lfor "Length" in 1D operations) in both declarations.Deeploy/Targets/PULPOpen/TileConstraints/ConvTileConstraint.py (4)
618-618: Remove unused variabledilation.The
dilationvariable is assigned but never used inaddGeometricalConstraint. If dilation support is intended for future use, consider adding a TODO comment; otherwise, remove it.♻️ Suggested fix
- dilation = parseDict["dilations"][0] + # Note: dilation not currently used in 1D constraint; add if needed
651-658: Remove unused variablepadsinaddPolicyConstraint.The
padsvariable is assigned on line 657 but never used in this method.♻️ Suggested fix
inputBuffer = ctxt.lookup(name = parseDict['data_in']) weightBuffer = ctxt.lookup(name = parseDict['weight']) - pads = parseDict["pads"] stride = parseDict["strides"][0]
768-769: Consider addingstrict=Truetozip()calls.Using
zip()withoutstrict=Truemay silently truncate if the input lists have different lengths, potentially masking bugs.♻️ Suggested fix
- for a, b, add, mul in zip(inputInCubes, inputWeightCubes, inputAddCubes, inputMulCubes): + for a, b, add, mul in zip(inputInCubes, inputWeightCubes, inputAddCubes, inputMulCubes, strict=True):
1046-1056: Consider addingstrict=Truetozip()calls.Same recommendation as in
RQConv1DTileConstraintfor defensive programming.♻️ Suggested fix
if varBias == "NULL": - for a, b in zip(inputInCubes, inputWeightCubes): + for a, b in zip(inputInCubes, inputWeightCubes, strict=True): inputLoadSchedule.append({"data_in": a, "weight": b}) else: - for a, b, c in zip(inputInCubes, inputWeightCubes, inputBiasCubes): + for a, b, c in zip(inputInCubes, inputWeightCubes, inputBiasCubes, strict=True): inputLoadSchedule.append({"data_in": a, "weight": b, "bias": c})
| effectiveInputLengthVar = inputLengthVar \ | ||
| + (sum(pads) * (inputLengthVar == inputBuffer.shape[1])) \ | ||
| - ((inputLengthVar - 1) * (inputLengthVar != inputBuffer.shape[1])) | ||
|
|
||
| _outputLengthVar = (effectiveInputLengthVar - dilation * (weightLengthVar - 1) - 1) // stride + 1 |
There was a problem hiding this comment.
Potential bug in effectiveInputLengthVar formula.
The formula subtracts (inputLengthVar - 1) when tiling, but comparing with Conv2DTileConstraint.addGeometricalConstraint (lines 305-308), the 2D version subtracts (weightHeightVar - 1). This appears to be a copy-paste error—it should likely be (weightLengthVar - 1) to account for the kernel receptive field overlap.
🐛 Suggested fix
# Assume worst case scenario (data padding on all sides) when tiling on a ceratin dimension.
effectiveInputLengthVar = inputLengthVar \
+ (sum(pads) * (inputLengthVar == inputBuffer.shape[1])) \
- - ((inputLengthVar - 1) * (inputLengthVar != inputBuffer.shape[1]))
+ - ((weightLengthVar - 1) * (inputLengthVar != inputBuffer.shape[1]))🤖 Prompt for AI Agents
In `@Deeploy/Targets/PULPOpen/TileConstraints/ConvTileConstraint.py` around lines
840 - 844, The effectiveInputLengthVar calculation is wrong: it subtracts
(inputLengthVar - 1) for the tiled case but should subtract the kernel span
(weightLengthVar - 1); update the expression in ConvTileConstraint (the
effectiveInputLengthVar assignment) to replace the final term ((inputLengthVar -
1) * (inputLengthVar != inputBuffer.shape[1])) with ((weightLengthVar - 1) *
(inputLengthVar != inputBuffer.shape[1])) so the receptive field overlap uses
weightLengthVar consistently with the 2D implementation.
| effectiveInputLength = inputLengthVar \ | ||
| + (sum(pads) * (inputLengthVar == inputBuffer.shape[1])) \ | ||
| - (inputLengthVar - 1) * (inputLengthVar != inputBuffer.shape[1]) |
There was a problem hiding this comment.
Same formula issue in addPolicyConstraint.
This has the same issue as addGeometricalConstraint—it subtracts (inputLengthVar - 1) instead of the kernel length minus one.
🐛 Suggested fix
effectiveInputLength = inputLengthVar \
+ (sum(pads) * (inputLengthVar == inputBuffer.shape[1])) \
- - (inputLengthVar - 1) * (inputLengthVar != inputBuffer.shape[1])
+ - (weightLengthVar - 1) * (inputLengthVar != inputBuffer.shape[1])🤖 Prompt for AI Agents
In `@Deeploy/Targets/PULPOpen/TileConstraints/ConvTileConstraint.py` around lines
871 - 873, The effectiveInputLength calculation in addPolicyConstraint uses
(inputLengthVar - 1) where it should subtract the kernel length minus one (same
bug as in addGeometricalConstraint); update the ternary expression so the last
term uses the kernel size (e.g., kernelLength or kernelLengthVar) minus one
instead of inputLengthVar - 1, i.e., change "- (inputLengthVar - 1) *
(inputLengthVar != inputBuffer.shape[1])" to "- (kernelLength - 1) *
(inputLengthVar != inputBuffer.shape[1])" in the effectiveInputLength assignment
inside addPolicyConstraint.
Victor-Jung
left a comment
Victor-Jung
left a comment
There was a problem hiding this comment.
Ciao Alex, I have a few minor comments but overall it looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Deeploy/Targets/Generic/Parsers.py (1)
193-200: PotentialKeyErrorwhenstridesattribute is missing.The stride validation on line 199 accesses
node.attrs['strides']unconditionally. Since this is a list literal passed toall(), Python evaluates all elements before callingall()— no short-circuit evaluation occurs. If'strides' not in node.attrs, this will raise aKeyError.🐛 Proposed fix: move stride validation after the attribute existence check
def parseNode(self, node: gs.Node) -> bool: ret = all([ 'ceil_mode' in node.attrs, 'kernel_shape' in node.attrs, 'pads' in node.attrs, 'strides' in node.attrs, len(node.inputs) == 1, len(node.outputs) >= 1, - all([stride > 0 for stride in node.attrs['strides']]), ]) + if ret: + ret = all(stride > 0 for stride in node.attrs['strides']) + if ret: self.operatorRepresentation['ceil_mode'] = node.attrs['ceil_mode']
🤖 Fix all issues with AI agents
In `@TargetLibraries/Generic/src/MaxPool_s8.c`:
- Around line 55-58: The code lacks a guard for K == 0 which allows a
zero-length kernel and produces invalid outputs; update the input validation in
MaxPool_s8 (the block checking L, K, S) to treat K == 0 as invalid—mirror the
existing stride guard (S == 0) by returning (or asserting) when K == 0 (e.g.,
add K == 0 to the condition that currently checks L < K || S == 0) so the
computation of L_out is never performed with a zero kernel.
|
I handled both conditions directly in the C kernel by early-returning when the parameters are invalid. I couldn’t find an existing pattern for this in other kernels, so this was a best-effort approach on my side — please let me know if there’s a preferred or more idiomatic way to handle it. Should I apply the same kind of guard to MaxPool2d as well? Additionally, since the stride value is available from ONNX, I added a guard in the parser to catch stride == 0. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Deeploy/Targets/Generic/Parsers.py (1)
195-211: KeyError whenstridesattribute is missing.The stride validation on line 202 accesses
node.attrs['strides']unconditionally. Sinceall([...])evaluates all list elements before callingall(), this will raise aKeyErrorif the'strides'attribute is not present—even though the check'strides' in node.attrsexists earlier in the list.🐛 Proposed fix: Move stride validation after initial checks
def parseNode(self, node: gs.Node) -> bool: ret = all([ 'ceil_mode' in node.attrs, 'kernel_shape' in node.attrs, 'pads' in node.attrs, 'strides' in node.attrs, len(node.inputs) == 1, len(node.outputs) >= 1, - all([stride > 0 for stride in node.attrs['strides']]), ]) - if ret: + if ret and not all(stride > 0 for stride in node.attrs['strides']): + return False + + if ret: self.operatorRepresentation['ceil_mode'] = node.attrs['ceil_mode'] self.operatorRepresentation['pads'] = node.attrs['pads'] self.operatorRepresentation['kernel_shape'] = node.attrs['kernel_shape'] self.operatorRepresentation['strides'] = node.attrs['strides'] return ret
🤖 Fix all issues with AI agents
In `@TargetLibraries/Generic/src/MaxPool_fp32.c`:
- Around line 46-50: The guard that validates input parameters in MaxPool_fp32.c
should also treat a kernel size of zero as invalid; update the condition
currently checking "if (W < K || S == 0)" to include "K == 0" so the function
returns early for empty kernels (this affects the validation in the function
containing the W_out calculation and the loop "for (uint32_t c = 0; c < C;
++c)"). Ensure the check prevents proceeding when K == 0 to avoid leaving max
initialized at -inf and producing invalid outputs.
♻️ Duplicate comments (1)
TargetLibraries/Generic/src/MaxPool_s8.c (1)
55-58: Still missing guard forK == 0.As flagged in a previous review, a zero-length kernel bypasses the current guard, causes the inner loop to never execute, and writes
-128 + output_offsetto all output positions. AddK == 0to the validation.Proposed fix
- if (L < K || S == 0) { + if (K == 0 || S == 0 || L < K) { return; }
Sorry, I missed the guard statement 😁 Sure, if you can add the guard to the other maxpool kernels, it would be amazing! Thanks for the contribution. I am approving this PR, and we can merge it soon. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Deeploy/Targets/Generic/Parsers.py`:
- Around line 195-203: The current boolean expression assigned to ret eagerly
accesses node.attrs['strides'] and can raise KeyError; modify the condition so
the positivity check runs only when 'strides' is present (e.g., replace the
inner all([stride > 0 for stride in node.attrs['strides']]) with a
short-circuiting check like ('strides' in node.attrs and all(stride > 0 for
stride in node.attrs['strides'])) or restructure the overall predicate to use
generator expressions that only evaluate node.attrs['strides'] after confirming
'strides' in node.attrs); update the expression that sets ret (and reference
node.attrs, 'strides', and the ret assignment) accordingly so missing strides
yields False without an exception.
In `@TargetLibraries/Generic/src/MaxPool_fp32.c`:
- Around line 14-16: The early-return in MaxPool_fp32.c currently checks if (H <
P || W < Q || SP == 0 || SQ == 0) but misses guarding P == 0 || Q == 0, which
allows empty pooling windows to leave max as -inf; update the condition in the
MaxPool_fp32 function to also return when P == 0 or Q == 0 (i.e., include P == 0
|| Q == 0 alongside the existing H, W, SP, SQ checks) so no invalid -inf outputs
are written.
🧹 Nitpick comments (1)
TargetLibraries/Generic/src/MaxPool_s8.c (1)
13-15: Add guards forP == 0andQ == 0.For consistency with the 1D variant feedback, a zero kernel size (
P == 0orQ == 0) would cause the inner loops to never execute, leavingmax = -128and producing semantically incorrect output. Consider guarding these cases as well.♻️ Proposed fix
- if (H < P || W < Q || SP == 0 || SQ == 0) { + if (P == 0 || Q == 0 || SP == 0 || SQ == 0 || H < P || W < Q) { return; }
| ret = all([ | ||
| 'ceil_mode' in node.attrs, 'kernel_shape' in node.attrs, 'pads' in node.attrs, 'strides' in node.attrs, | ||
| 'ceil_mode' in node.attrs, | ||
| 'kernel_shape' in node.attrs, | ||
| 'pads' in node.attrs, | ||
| 'strides' in node.attrs, | ||
| len(node.inputs) == 1, | ||
| len(node.outputs) >= 1 | ||
| len(node.outputs) >= 1, | ||
| all([stride > 0 for stride in node.attrs['strides']]), | ||
| ]) |
There was a problem hiding this comment.
Avoid KeyError when strides is absent (Line 195–203).
all([...]) eagerly evaluates the list, so node.attrs['strides'] will throw if the attribute is missing; previously this would just return False. Guard the access and run the positivity check only when strides is present.
🛠️ Proposed fix
- ret = all([
- 'ceil_mode' in node.attrs,
- 'kernel_shape' in node.attrs,
- 'pads' in node.attrs,
- 'strides' in node.attrs,
- len(node.inputs) == 1,
- len(node.outputs) >= 1,
- all([stride > 0 for stride in node.attrs['strides']]),
- ])
+ strides = node.attrs.get('strides')
+ ret = all([
+ 'ceil_mode' in node.attrs,
+ 'kernel_shape' in node.attrs,
+ 'pads' in node.attrs,
+ strides is not None,
+ len(node.inputs) == 1,
+ len(node.outputs) >= 1,
+ ])
+ if ret and not all(stride > 0 for stride in strides):
+ return False🤖 Prompt for AI Agents
In `@Deeploy/Targets/Generic/Parsers.py` around lines 195 - 203, The current
boolean expression assigned to ret eagerly accesses node.attrs['strides'] and
can raise KeyError; modify the condition so the positivity check runs only when
'strides' is present (e.g., replace the inner all([stride > 0 for stride in
node.attrs['strides']]) with a short-circuiting check like ('strides' in
node.attrs and all(stride > 0 for stride in node.attrs['strides'])) or
restructure the overall predicate to use generator expressions that only
evaluate node.attrs['strides'] after confirming 'strides' in node.attrs); update
the expression that sets ret (and reference node.attrs, 'strides', and the ret
assignment) accordingly so missing strides yields False without an exception.
… and float MaxPool1D
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@Deeploy/Targets/PULPOpen/TileConstraints/ConvTileConstraint.py`:
- Around line 616-630: Remove or silence the three unused local variables to
satisfy Ruff F841: eliminate the unused assignments to dilation,
inputChannelVar, and weightInChannelVar (or rename them to _dilation,
_inputChannelVar, _weightInChannelVar) in the method that builds the tiler model
where tilerModel.addTensorDimToModel(...) and tilerModel.getTensorDimVar(...)
are called; ensure any remaining needed dimension variables (e.g.,
inputBatchVar, inputLengthVar, weightOutChannelVar, weightLengthVar) are left
intact and no other logic depends on the removed variables.
- Around line 657-658: Remove the unused variable assignment pads =
parseDict["pads"] in ConvTileConstraint.py to satisfy Ruff F841; locate the
occurrence where parseDict is read (the pads assignment near stride =
parseDict["strides"][0]) and either delete that pads line entirely or, if the
read is needed for side effects, replace it with an intentional discard like _ =
parseDict["pads"] so the parser intent remains clear.
- Around line 768-769: Update all zip() calls in ConvTileConstraint.py to pass
strict=True to satisfy B905: add strict=True to the zip used to build
inputLoadSchedule (the loop that zips inputInCubes, inputWeightCubes,
inputAddCubes, inputMulCubes producing inputLoadSchedule), and likewise add
strict=True to the zip calls in the other locations mentioned (the zip usages
around lines where input/weight/add/mul cube lists are zipped and the ones at
lines 221, 583, 586, 1048, and 1051). Locate the loops/functions that use zip
(e.g., the function/method that fills inputLoadSchedule and any helper functions
that iterate over cube lists) and change zip(...) to zip(..., strict=True) for
each occurrence so mismatched-length iterables raise immediately.
| pads = parseDict["pads"] | ||
| stride = parseDict["strides"][0] | ||
| dilation = parseDict["dilations"][0] | ||
|
|
||
| # Add I/O dimensions to the model as variables | ||
| for bufferName in [inputBufferName, weightBufferName, mulBufferName, addBufferName, outputBufferName]: | ||
| tilerModel.addTensorDimToModel(ctxt, bufferName) | ||
|
|
||
| inputBatchVar = tilerModel.getTensorDimVar(tensorName = inputBufferName, dimIdx = 0) | ||
| inputLengthVar = tilerModel.getTensorDimVar(tensorName = inputBufferName, dimIdx = 1) | ||
| inputChannelVar = tilerModel.getTensorDimVar(tensorName = inputBufferName, dimIdx = 2) | ||
|
|
||
| weightOutChannelVar = tilerModel.getTensorDimVar(tensorName = weightBufferName, dimIdx = 0) | ||
| weightLengthVar = tilerModel.getTensorDimVar(tensorName = weightBufferName, dimIdx = 1) | ||
| weightInChannelVar = tilerModel.getTensorDimVar(tensorName = weightBufferName, dimIdx = 2) |
There was a problem hiding this comment.
Remove unused locals to satisfy Ruff F841.
dilation, inputChannelVar, and weightInChannelVar are assigned but unused in this method.
🧹 Proposed fix
- dilation = parseDict["dilations"][0]
-
# Add I/O dimensions to the model as variables
for bufferName in [inputBufferName, weightBufferName, mulBufferName, addBufferName, outputBufferName]:
tilerModel.addTensorDimToModel(ctxt, bufferName)
@@
- inputChannelVar = tilerModel.getTensorDimVar(tensorName = inputBufferName, dimIdx = 2)
-
weightOutChannelVar = tilerModel.getTensorDimVar(tensorName = weightBufferName, dimIdx = 0)
weightLengthVar = tilerModel.getTensorDimVar(tensorName = weightBufferName, dimIdx = 1)
- weightInChannelVar = tilerModel.getTensorDimVar(tensorName = weightBufferName, dimIdx = 2)🧰 Tools
🪛 Ruff (0.14.14)
[error] 618-618: Local variable dilation is assigned to but never used
Remove assignment to unused variable dilation
(F841)
[error] 626-626: Local variable inputChannelVar is assigned to but never used
Remove assignment to unused variable inputChannelVar
(F841)
[error] 630-630: Local variable weightInChannelVar is assigned to but never used
Remove assignment to unused variable weightInChannelVar
(F841)
🤖 Prompt for AI Agents
In `@Deeploy/Targets/PULPOpen/TileConstraints/ConvTileConstraint.py` around lines
616 - 630, Remove or silence the three unused local variables to satisfy Ruff
F841: eliminate the unused assignments to dilation, inputChannelVar, and
weightInChannelVar (or rename them to _dilation, _inputChannelVar,
_weightInChannelVar) in the method that builds the tiler model where
tilerModel.addTensorDimToModel(...) and tilerModel.getTensorDimVar(...) are
called; ensure any remaining needed dimension variables (e.g., inputBatchVar,
inputLengthVar, weightOutChannelVar, weightLengthVar) are left intact and no
other logic depends on the removed variables.
| pads = parseDict["pads"] | ||
| stride = parseDict["strides"][0] |
There was a problem hiding this comment.
Remove unused pads to satisfy Ruff F841.
🧹 Proposed fix
- pads = parseDict["pads"]
stride = parseDict["strides"][0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pads = parseDict["pads"] | |
| stride = parseDict["strides"][0] | |
| stride = parseDict["strides"][0] |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 657-657: Local variable pads is assigned to but never used
Remove assignment to unused variable pads
(F841)
🤖 Prompt for AI Agents
In `@Deeploy/Targets/PULPOpen/TileConstraints/ConvTileConstraint.py` around lines
657 - 658, Remove the unused variable assignment pads = parseDict["pads"] in
ConvTileConstraint.py to satisfy Ruff F841; locate the occurrence where
parseDict is read (the pads assignment near stride = parseDict["strides"][0])
and either delete that pads line entirely or, if the read is needed for side
effects, replace it with an intentional discard like _ = parseDict["pads"] so
the parser intent remains clear.
| for a, b, add, mul in zip(inputInCubes, inputWeightCubes, inputAddCubes, inputMulCubes): | ||
| inputLoadSchedule.append({"data_in": a, "weight": b, "add": add, "mul": mul}) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find Python version requirements in the project
find . -type f \( -name "setup.py" -o -name "setup.cfg" -o -name "pyproject.toml" -o -name ".python-version" -o -name "tox.ini" -o -name "requirements*.txt" \) | head -20Repository: pulp-platform/Deeploy
Length of output: 115
🏁 Script executed:
# Check the file and locate the code at the mentioned lines
wc -l Deeploy/Targets/PULPOpen/TileConstraints/ConvTileConstraint.pyRepository: pulp-platform/Deeploy
Length of output: 132
🏁 Script executed:
# Read the relevant sections of the file
sed -n '765,775p' Deeploy/Targets/PULPOpen/TileConstraints/ConvTileConstraint.py
sed -n '1045,1055p' Deeploy/Targets/PULPOpen/TileConstraints/ConvTileConstraint.pyRepository: pulp-platform/Deeploy
Length of output: 1084
🏁 Script executed:
# Check Python version requirement
cat setup.py | grep -A 5 "python_requires"Repository: pulp-platform/Deeploy
Length of output: 47
🏁 Script executed:
# Also check pyproject.toml
cat pyproject.toml | grep -A 5 "python"Repository: pulp-platform/Deeploy
Length of output: 166
🌐 Web query:
B905 flake8 rule zip strict parameter Python
💡 Result:
B905 (from flake8-bugbear) flags uses of zip() with 2+ iterables (or any starred *args) that don’t pass an explicit strict= keyword. The intent is to prevent subtle bugs from zip silently truncating to the shortest iterable. Set either:
strict=Trueto raiseValueErrorif lengths differ (safer), orstrict=Falseto keep truncation but make it explicit.
Notes:
strict=exists only in Python 3.10+, so don’t enable B905 if you need to support Python <3.10.itertools.count,itertools.cycle, anditertools.repeat(times=None)are excluded as infinite iterators.
Sources: flake8-bugbear docs on PyPI [1] (also mirrored elsewhere [2]); Ruff’s equivalent rule description [3].
🏁 Script executed:
# Search for all zip() calls in the file to ensure the review comment covers all instances
grep -n "zip(" Deeploy/Targets/PULPOpen/TileConstraints/ConvTileConstraint.pyRepository: pulp-platform/Deeploy
Length of output: 566
Add explicit strict= to zip() calls to satisfy B905.
The project requires Python 3.10+, which natively supports the strict= parameter. Add strict=True to the following zip() calls:
- Lines 768-769, 1048, and 1051 (as shown in the proposed fix)
- Also lines 221, 583, and 586 have similar patterns that should be updated consistently
Proposed fix for lines 768-769, 1048, 1051
- for a, b, add, mul in zip(inputInCubes, inputWeightCubes, inputAddCubes, inputMulCubes):
+ for a, b, add, mul in zip(inputInCubes, inputWeightCubes, inputAddCubes, inputMulCubes, strict=True):
inputLoadSchedule.append({"data_in": a, "weight": b, "add": add, "mul": mul})- for a, b in zip(inputInCubes, inputWeightCubes):
+ for a, b in zip(inputInCubes, inputWeightCubes, strict=True):
inputLoadSchedule.append({"data_in": a, "weight": b})- for a, b, c in zip(inputInCubes, inputWeightCubes, inputBiasCubes):
+ for a, b, c in zip(inputInCubes, inputWeightCubes, inputBiasCubes, strict=True):
inputLoadSchedule.append({"data_in": a, "weight": b, "bias": c})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for a, b, add, mul in zip(inputInCubes, inputWeightCubes, inputAddCubes, inputMulCubes): | |
| inputLoadSchedule.append({"data_in": a, "weight": b, "add": add, "mul": mul}) | |
| for a, b, add, mul in zip(inputInCubes, inputWeightCubes, inputAddCubes, inputMulCubes, strict=True): | |
| inputLoadSchedule.append({"data_in": a, "weight": b, "add": add, "mul": mul}) |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 768-768: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
In `@Deeploy/Targets/PULPOpen/TileConstraints/ConvTileConstraint.py` around lines
768 - 769, Update all zip() calls in ConvTileConstraint.py to pass strict=True
to satisfy B905: add strict=True to the zip used to build inputLoadSchedule (the
loop that zips inputInCubes, inputWeightCubes, inputAddCubes, inputMulCubes
producing inputLoadSchedule), and likewise add strict=True to the zip calls in
the other locations mentioned (the zip usages around lines where
input/weight/add/mul cube lists are zipped and the ones at lines 221, 583, 586,
1048, and 1051). Locate the loops/functions that use zip (e.g., the
function/method that fills inputLoadSchedule and any helper functions that
iterate over cube lists) and change zip(...) to zip(..., strict=True) for each
occurrence so mismatched-length iterables raise immediately.
3 participants
Add integer MaxPool1D for Generic platform and RQSConv1D support for PULPOpen, with corresponding kernel tests.
Added
Changed
Fixed
PR Merge Checklist
develcommit and pointing todevel.CHANGELOG.mdfile has been updated.