Improve Docstring and Debugging #160
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughReorganizes memory-aware passes into a new MemoryLevelExtension, converts Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Xeratec
force-pushed
the
pr/docs-and-debug
branch
from
d14f8cb to
092950d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Deeploy/CommonExtensions/DataTypes.py (1)
68-98:⚠️ Potential issue | 🟡 MinorFix float64_t docstring mantissa/exponent swap (Line 93).
The docstring currently inverts mantissa/exponent widths relative to the class attributes.Proposed fix
-class float64_t(FloatImmediate): - """64-bit float type with 11-bit mantissa and 52-bit exponent.""" +class float64_t(FloatImmediate): + """64-bit float type with 52-bit mantissa and 11-bit exponent."""Deeploy/Targets/PULPOpen/Bindings.py (1)
63-75:⚠️ Potential issue | 🟡 MinorDuplicate closure definitions detected.
FunctionCallClosureandForkClosureare defined twice:
- Lines 63-69: First definitions
- Lines 71-75: Duplicate definitions that overwrite the first
This appears to be an unintentional artifact from the refactoring. Please remove the duplicate definitions.
🔧 Proposed fix to remove duplicates
FunctionCallClosure = partial(ClosureGeneration, closureSuffix = "_closure") ClusterClosure = partial(ClosureGeneration, closureSuffix = "_cluster_entry", closureCallTemplate = _clusterEntryClosureCallTemplate) ForkClosure = partial(ClosureGeneration, closureSuffix = "_cluster_fork", closureCallTemplate = _clusterForkClosureCallTemplate) TilingCallClosure = partial(ClosureGeneration, closureSuffix = "_tiling_closure") -FunctionCallClosure = partial(ClosureGeneration, closureSuffix = "_closure") -ForkClosure = partial(ClosureGeneration, - closureSuffix = "_cluster_fork", - closureCallTemplate = _clusterForkClosureCallTemplate) MemoryAwareClusterClosure = partial(MemoryAwareClosureGeneration,Deeploy/CommonExtensions/OptimizationPasses/Matchers.py (2)
367-390:⚠️ Potential issue | 🟠 MajorGuard against leaf nodes before indexing outputs.
When the graph node is a leaf but the pattern expects more nodes,gn.outputs[0]/pn.outputs[0]will raiseIndexError. Short‑circuit when outputs are missing.🛠️ Proposed fix
# End of pattern if pattern_length == 1: return nodes_map + # If either graph or pattern stops here, it can't match a longer chain + if len(gn.outputs) == 0 or len(pn.outputs) == 0: + return None + # if we are in the "active" pattern, the graph node has to be # single-output and single-use
581-633:⚠️ Potential issue | 🟠 MajorAvoid in-place nodes_map contamination during branch backtracking.
Reusing the same dict across alternative branch attempts can leave partial mappings after a failed recursion and block valid matches. Clone per attempt and only commit on success.🛠️ Proposed fix
if len(pn_input.inputs) > 0 and pn_input.inputs[0].name not in nodes_map.keys(): tmp = None for gn_input in gn.inputs: # Check if parent node of gn is constant or input node (in this case it has no additional inputs) # and if node was already matched if len(gn_input.inputs) > 0 and gn_input.inputs[0] not in nodes_map.values(): # Search for valid subgraphs - tmp = self._match_nodes_recursive(pn_input.inputs[0], - gn_input.inputs[0], - nodes_map, - direction = 'Reverse') - if tmp is not None: - nodes_map = tmp + candidate_map = dict(nodes_map) + tmp = self._match_nodes_recursive(pn_input.inputs[0], + gn_input.inputs[0], + candidate_map, + direction='Reverse') + if tmp is not None: + nodes_map = tmp + break @@ if len(pn_input.outputs) > 0 and pn_input.outputs[0].name not in nodes_map.keys(): tmp = None for gn_input in gn.outputs: # Check if parent node of gn is is output node (in this case it has no additional outputs) # and if node was already matched if len(gn_input.outputs) > 0 and gn_input.outputs[0] not in nodes_map.values(): # Search for valid subgraphs - tmp = self._match_nodes_recursive(pn_input.outputs[0], - gn_input.outputs[0], - nodes_map, - direction = 'Forward') - if tmp is not None: - nodes_map = tmp + candidate_map = dict(nodes_map) + tmp = self._match_nodes_recursive(pn_input.outputs[0], + gn_input.outputs[0], + candidate_map, + direction='Forward') + if tmp is not None: + nodes_map = tmp + breakDeeploy/CommonExtensions/CodeTransformationPasses/MemoryAllocation.py (1)
172-213:⚠️ Potential issue | 🟠 MajorGuard against already-sorted buffers in topological sort loop.
The while loop iterates through all buffers multiple times, but the for loop processes each buffer without checking if it was already removed in a previous iteration. When a buffer is removed from
unsortedBufferNamesin one pass and encountered again in a subsequent pass, calling.remove()on an already-removed name raisesValueError. Add the guard check before attempting removal.Proposed fix
while len(unsortedBufferNames) > 0: for buffer in buffers: + if buffer.name not in unsortedBufferNames: + continue if isinstance(buffer, _ReferenceBuffer) and buffer._referenceName in unsortedBufferNames: continue sortedBuffers.append(buffer) unsortedBufferNames.remove(buffer.name)
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 37: Fix the typo in the CHANGELOG entry: change "calss" to "class" in the
line that reads "Add `__repr__()` function for `_ReferenceBuffer` calss" so it
reads "Add `__repr__()` function for `_ReferenceBuffer` class"; update the text
around the `_ReferenceBuffer` and `__repr__()` mention if necessary to preserve
formatting consistency.
In
`@Deeploy/CommonExtensions/CodeTransformationPasses/IntrospectiveCodeTransformation.py`:
- Line 42: parseTreeDict is a mutable class attribute and should be annotated as
typing.ClassVar to make its shared, class-level nature explicit; update the
declaration of parseTreeDict (the class attribute named parseTreeDict: Dict[int,
TemplateNode]) to use ClassVar[Dict[int, TemplateNode]] and add/import ClassVar
from typing in the module so linters and readers see it's intentionally shared
across instances.
In `@Deeploy/CommonExtensions/CodeTransformationPasses/MemoryAllocation.py`:
- Around line 257-260: The allocation loop is reversing the output of
topologicallySortBuffers which already yields dependencies first, so using
reversed(...) causes dependents to be allocated before their references; remove
the reversed(...) call and iterate directly over
self.topologicallySortBuffers(outputs + transients) in the allocation loop (the
block that asserts buffer._live is False and allocates buffers referenced by
buffer.name) so reference buffers are allocated prior to their dependents.
In `@Deeploy/CommonExtensions/DataTypes.py`:
- Around line 136-148: The docstring for minimalFloatType has a parameter
name/type mismatch and a missing bracket: change the function signature and
docstring to consistently use "values" (not "value") and correct the type
annotation to Union[float, Iterable[float], npt.NDArray] (ensure the closing
bracket is present), then update the "Parameters" section to refer to "values"
and include the complete type expression (closing bracket/NDArray) so it matches
minimalIntegerType's style and the actual signature.
- Around line 110-122: The docstring for minimalIntegerType mismatches the
function signature: change the Parameters section to refer to "value" (singular)
and fix the type annotation to match the function signature (Union[int,
Iterable[int], npt.NDArray]) including the missing closing bracket and NDArray
type; also ensure the param description reflects that a single int, an iterable
of ints, or a numpy NDArray may be passed, and keep the Returns section as
Type[IntegerImmediate] to match the function's return type.
In `@Deeploy/DeeployTypes.py`:
- Around line 390-396: The fromVariableBuffer classmethod constructs a
TransientBuffer instance into the local variable ret (using cls(name =
buffer.name, size = buffer.sizeInBytes)) but never returns it; update the method
(fromVariableBuffer) to return ret (i.e., add a return ret at the end) so
callers receive the constructed instance when passing a VariableBuffer.
In `@Deeploy/Targets/Neureka/OptimizationPasses/__init__.py`:
- Line 24: The package __init__.py currently uses "from . import *" which
doesn't import submodule symbols; replace it by explicitly importing the class
from the submodule and re-exporting it so callers can do "from
Deeploy.Targets.Neureka.OptimizationPasses import
AnnotateNeurekaWeightMemoryLevel": import AnnotateNeurekaWeightMemoryLevel from
the MemoryLevelAnnotationPasses module (referencing MemoryLevelAnnotationPasses
and AnnotateNeurekaWeightMemoryLevel) and set __all__ to include
'AnnotateNeurekaWeightMemoryLevel'.
In `@Deeploy/Targets/Neureka/OptimizationPasses/MemoryLevelAnnotationPasses.py`:
- Line 53: The list comprehension creating neurekaNodes assumes every node has
an "engine" key and can raise KeyError; change the membership test to use safe
lookup (e.g., node.attrs.get("engine") or node.attrs.get("engine", None")) when
comparing to self.neurekaEngineName so nodes missing "engine" are treated as
non-matching; update the comprehension that builds neurekaNodes (referencing
neurekaNodes, graph.nodes, node.attrs, and self.neurekaEngineName) accordingly.
In `@Deeploy/TilingExtension/TilerExtension.py`:
- Around line 46-91: The docstring example for class Tiler uses an undefined
variable `hierarchy` when constructing the example instance; update the example
to use the defined variable name (`memoryHierarchy`) or define `hierarchy`
explicitly so it matches the earlier example variables—e.g. change `tiler =
Tiler(hierarchy)` to `tiler = Tiler(memoryHierarchy)` in the Tiler class
docstring.
- Around line 1792-1809: The docstring for the worstCaseBufferSize property
claims it includes input/output buffers but the current implementation returns
self.tiler.worstCaseBufferSize only; either update the implementation to
actually add IO sizes or soften the docstring. To fix, either (A) compute IO
buffer sizes from the class' IO sources (e.g., attributes or helper methods that
expose input/output sizes such as any inputBuffers/outputBuffers or
get_input_buffer_sizes/get_output_buffer_sizes), merge those per memory-level
with the dictionary returned by self.tiler.worstCaseBufferSize (summing values
for matching keys) and return the combined dict from worstCaseBufferSize, or (B)
change the worstCaseBufferSize docstring to state it delegates to
self.tiler.worstCaseBufferSize and does not include input/output buffers. Ensure
you modify the worstCaseBufferSize property accordingly and reference
self.tiler.worstCaseBufferSize in the implementation or docstring.
🧹 Nitpick comments (6)
Deeploy/CommonExtensions/CodeTransformationPasses/IntrospectiveCodeTransformation.py (1)
42-42: Consider adding cache size management.The
parseTreeDictcache has no size limit or eviction policy. While this may not be an issue for typical workloads with a limited number of unique templates, it could lead to unbounded memory growth in long-running processes that generate many unique templates.Consider implementing a cache size limit (e.g., using
functools.lru_cachepattern or a bounded dictionary) if memory consumption becomes a concern in production environments.Deeploy/Targets/Neureka/OptimizationPasses/MemoryLevelAnnotationPasses.py (1)
61-63: Boundary condition uses<instead of<=.A buffer that would exactly fill the remaining memory capacity is excluded. If this is intentional (e.g., reserving some headroom), consider adding a comment. Otherwise, using
<=would allow buffers that exactly fit.Suggested change if exact fit should be allowed
- if weightMemoryOccupation + _neurekaWeightBufferSize(buffer) < self._weightMemoryLevel.size: + if weightMemoryOccupation + _neurekaWeightBufferSize(buffer) <= self._weightMemoryLevel.size:Deeploy/MemoryLevelExtension/CodeTransformationPasses/Closure.py (2)
79-79: Misleading comment: "Don't override this".This method is an override of the base class's
_generateClosureStruct. The comment likely means "subclasses of this class should not override this method," but it reads as if the current implementation shouldn't override the parent. Consider clarifying.Suggested clarification
- # Don't override this + # Subclasses should not override this method def _generateClosureStruct(self, ctxt: NetworkContext, executionBlock: ExecutionBlock):
120-121: Filter condition can be simplified.The condition
buf._memoryLevel == self.startRegion or buf._memoryLevel != self.endRegionis equivalent tobuf._memoryLevel != self.endRegion(assumingstartRegion != endRegion). ThestartRegioncheck is only meaningful ifstartRegion == endRegion, which would be an unusual configuration.If the intent is simply "exclude endRegion buffers," the condition could be simplified:
Simplified filter condition
- if buf._memoryLevel == self.startRegion or buf._memoryLevel != self.endRegion: + if buf._memoryLevel != self.endRegion:If the explicit
startRegioncheck serves a purpose (e.g., documentation or future extensibility), consider adding a comment explaining why.Deeploy/MemoryLevelExtension/CodeTransformationPasses/__init__.py (1)
24-24:from . import *does not import submodules; consider explicit imports for better code organization.While this pattern technically doesn't expose
MemoryAwareClosureGeneration,MemoryAwarePrintInputGeneration, orMemoryAwarePrintOutputGenerationvia the package namespace, the codebase imports these classes directly from their submodules (e.g.,from ...CodeTransformationPasses.Closure import MemoryAwareClosureGeneration), so there is no current functional issue. For improved code clarity and package organization, consider using explicit imports with__all__.Suggested improvement for explicit imports
-from . import * +from .Closure import MemoryAwareClosureGeneration +from .PrintInputs import MemoryAwarePrintInputGeneration, MemoryAwarePrintOutputGeneration + +__all__ = [ + "MemoryAwareClosureGeneration", + "MemoryAwarePrintInputGeneration", + "MemoryAwarePrintOutputGeneration", +]Deeploy/CommonExtensions/CodeTransformationPasses/MemoryAllocation.py (1)
310-372: Optional: silence unusedverboseif you keep Ruff clean.
This parameter is unused and triggers ARG002; consider a no-op assignment or a# noqa: ARG002.🧹 Example (no-op use)
def apply(self, ctxt: NetworkContext, executionBlock: ExecutionBlock, name: str, verbose: CodeGenVerbosity = _NoVerbosity) -> Tuple[NetworkContext, ExecutionBlock]: + _ = verbose references = self.extractDynamicReferences(ctxt, executionBlock, unrollStructs = True, includeGlobalReferences = False)
Deeploy/CommonExtensions/CodeTransformationPasses/IntrospectiveCodeTransformation.py
Show resolved
Hide resolved
| def minimalIntegerType(value: Union[int, Iterable[int], npt.NDArray]) -> Type[IntegerImmediate]: | ||
| """Returns the minimal integer type that can represent all values in the given list. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| values : Union[int, Iterable[int] | ||
| The list of integer values to analyze. | ||
|
|
||
| Returns | ||
| ------- | ||
| Type[IntegerImmediate] | ||
| The minimal integer type that can represent all values. | ||
| """ |
There was a problem hiding this comment.
Docstring parameter name/type mismatch in minimalIntegerType.
The docstring uses values (plural) and is missing the closing bracket and NDArray type.
Proposed fix
- Parameters
- ----------
- values : Union[int, Iterable[int]
- The list of integer values to analyze.
+ Parameters
+ ----------
+ value : Union[int, Iterable[int], npt.NDArray]
+ The integer value(s) to analyze.🤖 Prompt for AI Agents
In `@Deeploy/CommonExtensions/DataTypes.py` around lines 110 - 122, The docstring
for minimalIntegerType mismatches the function signature: change the Parameters
section to refer to "value" (singular) and fix the type annotation to match the
function signature (Union[int, Iterable[int], npt.NDArray]) including the
missing closing bracket and NDArray type; also ensure the param description
reflects that a single int, an iterable of ints, or a numpy NDArray may be
passed, and keep the Returns section as Type[IntegerImmediate] to match the
function's return type.
| def minimalFloatType(value: Union[float, Iterable[float], npt.NDArray]) -> Type[FloatImmediate]: | ||
| """Returns the minimal float type that can represent all values in the given list. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| values : Union[float, Iterable[float] | ||
| The list of float values to analyze. | ||
|
|
||
| Returns | ||
| ------- | ||
| Type[FloatImmediate] | ||
| The minimal float type that can represent all values. | ||
| """ |
There was a problem hiding this comment.
Docstring parameter name/type mismatch in minimalFloatType.
Same issue as minimalIntegerType: values vs value, and missing closing bracket/NDArray.
Proposed fix
- Parameters
- ----------
- values : Union[float, Iterable[float]
- The list of float values to analyze.
+ Parameters
+ ----------
+ value : Union[float, Iterable[float], npt.NDArray]
+ The float value(s) to analyze.📝 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.
| def minimalFloatType(value: Union[float, Iterable[float], npt.NDArray]) -> Type[FloatImmediate]: | |
| """Returns the minimal float type that can represent all values in the given list. | |
| Parameters | |
| ---------- | |
| values : Union[float, Iterable[float] | |
| The list of float values to analyze. | |
| Returns | |
| ------- | |
| Type[FloatImmediate] | |
| The minimal float type that can represent all values. | |
| """ | |
| def minimalFloatType(value: Union[float, Iterable[float], npt.NDArray]) -> Type[FloatImmediate]: | |
| """Returns the minimal float type that can represent all values in the given list. | |
| Parameters | |
| ---------- | |
| value : Union[float, Iterable[float], npt.NDArray] | |
| The float value(s) to analyze. | |
| Returns | |
| ------- | |
| Type[FloatImmediate] | |
| The minimal float type that can represent all values. | |
| """ |
🤖 Prompt for AI Agents
In `@Deeploy/CommonExtensions/DataTypes.py` around lines 136 - 148, The docstring
for minimalFloatType has a parameter name/type mismatch and a missing bracket:
change the function signature and docstring to consistently use "values" (not
"value") and correct the type annotation to Union[float, Iterable[float],
npt.NDArray] (ensure the closing bracket is present), then update the
"Parameters" section to refer to "values" and include the complete type
expression (closing bracket/NDArray) so it matches minimalIntegerType's style
and the actual signature.
| @classmethod | ||
| def fromVariableBuffer(cls, buffer: VariableBuffer): | ||
| ret = cls(name = buffer.name, size = buffer.sizeInBytes) | ||
|
|
||
| @property | ||
| def sizeInBytes(self) -> int: | ||
| return int(self.size) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "DeeployTypes.py" -type fRepository: pulp-platform/Deeploy
Length of output: 90
🏁 Script executed:
sed -n '385,400p' ./Deeploy/DeeployTypes.pyRepository: pulp-platform/Deeploy
Length of output: 607
Add return statement to fromVariableBuffer method.
The fromVariableBuffer classmethod constructs a ret instance but never returns it, causing the method to implicitly return None. Callers will receive None instead of the expected TransientBuffer instance.
Fix
`@classmethod`
def fromVariableBuffer(cls, buffer: VariableBuffer):
ret = cls(name = buffer.name, size = buffer.sizeInBytes)
+ return ret📝 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.
| @classmethod | |
| def fromVariableBuffer(cls, buffer: VariableBuffer): | |
| ret = cls(name = buffer.name, size = buffer.sizeInBytes) | |
| @property | |
| def sizeInBytes(self) -> int: | |
| return int(self.size) | |
| `@classmethod` | |
| def fromVariableBuffer(cls, buffer: VariableBuffer): | |
| ret = cls(name = buffer.name, size = buffer.sizeInBytes) | |
| return ret | |
| `@property` | |
| def sizeInBytes(self) -> int: | |
| return int(self.size) |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 392-392: Local variable ret is assigned to but never used
Remove assignment to unused variable ret
(F841)
🤖 Prompt for AI Agents
In `@Deeploy/DeeployTypes.py` around lines 390 - 396, The fromVariableBuffer
classmethod constructs a TransientBuffer instance into the local variable ret
(using cls(name = buffer.name, size = buffer.sizeInBytes)) but never returns it;
update the method (fromVariableBuffer) to return ret (i.e., add a return ret at
the end) so callers receive the constructed instance when passing a
VariableBuffer.
Deeploy/Targets/Neureka/OptimizationPasses/MemoryLevelAnnotationPasses.py
Show resolved
Hide resolved
Xeratec
force-pushed
the
pr/docs-and-debug
branch
from
092950d to
ae4525a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Deeploy/CommonExtensions/CodeTransformationPasses/MemoryAllocation.py (1)
172-213:⚠️ Potential issue | 🟠 Major
topologicallySortBufferscan re-process already-sorted buffers.
A second while-iteration can hitunsortedBufferNames.remove(buffer.name)for buffers already removed, raisingValueError.🛠️ Proposed fix
while len(unsortedBufferNames) > 0: for buffer in buffers: + if buffer.name not in unsortedBufferNames: + continue if isinstance(buffer, _ReferenceBuffer) and buffer._referenceName in unsortedBufferNames: continue sortedBuffers.append(buffer) unsortedBufferNames.remove(buffer.name)Deeploy/CommonExtensions/OptimizationPasses/Matchers.py (2)
498-633:⚠️ Potential issue | 🟠 MajorBacktracking uses shared
nodes_mapand can poison later candidates.Recursive calls mutate
nodes_mapin-place even when a candidate path fails, which can incorrectly block subsequent candidates and lead to false negatives. Use a copy per branch and only merge on success.🧩 Proposed fix (copy-on-branch)
- tmp = None + tmp = None for gn_input in gn.inputs: @@ - tmp = self._match_nodes_recursive(pn_input.inputs[0], - gn_input.inputs[0], - nodes_map, - direction = 'Reverse') + candidate_map = dict(nodes_map) + tmp = self._match_nodes_recursive(pn_input.inputs[0], + gn_input.inputs[0], + candidate_map, + direction = 'Reverse') if tmp is not None: nodes_map = tmp + break @@ - if tmp == None: + if tmp == None: return None @@ - tmp = None + tmp = None for gn_input in gn.outputs: @@ - tmp = self._match_nodes_recursive(pn_input.outputs[0], - gn_input.outputs[0], - nodes_map, - direction = 'Forward') + candidate_map = dict(nodes_map) + tmp = self._match_nodes_recursive(pn_input.outputs[0], + gn_input.outputs[0], + candidate_map, + direction = 'Forward') if tmp is not None: nodes_map = tmp + break @@ - if tmp == None: + if tmp == None: return None
198-248:⚠️ Potential issue | 🟡 MinorNon-overlap tracking depends on unique node names.
Line 239 uses
node.namefor overlap detection. If the graph contains nodes with duplicate names, legitimate matches could be incorrectly rejected or overlapping matches could be allowed. Consider tracking identity viaid(node)instead, or enforce name uniqueness upstream when constructing graphs.🛠️ Safer overlap tracking by identity
- matched_node_names = set() + matched_node_ids = set() - def node_names(match: Match): - return [node.name for node in match.nodes_map.values()] + def node_ids(match: Match): + return [id(node) for node in match.nodes_map.values()] def is_overlap(match: Match): - return not matched_node_names.isdisjoint(node_names(match)) + return not matched_node_ids.isdisjoint(node_ids(match)) @@ - matched_node_names.update(node_names(match)) + matched_node_ids.update(node_ids(match))
🤖 Fix all issues with AI agents
In `@Deeploy/MemoryLevelExtension/CodeTransformationPasses/__init__.py`:
- Line 24: Replace the wildcard import in
Deeploy.MemoryLevelExtension.CodeTransformationPasses.__init__ (currently "from
. import *") with explicit re-exports: import the public classes/functions from
their submodules (e.g., from .submodule_name import ClassName, function_name)
and then define __all__ = ["ClassName", "function_name", ...] so the package
namespace exposes the intended API; update the list to include every public
symbol you want exported from the package.
In `@Deeploy/MemoryLevelExtension/CodeTransformationPasses/PrintInputs.py`:
- Around line 110-119: The code calls
self.regex.findall(ctxt.lookup(key)._memoryLevel) without guarding against
_memoryLevel being None; change the logic in the PrintInputs method to use the
already-retrieved _buffer (from ctxt.lookup(key)), check hasattr(_buffer,
"_memoryLevel") and also if getattr(_buffer, "_memoryLevel") is None return
False when self.regex is set, and only call self.regex.findall on a non-None
string value (avoid calling ctxt.lookup(key) twice); ensure the returned boolean
remains ret != [].
In `@Deeploy/TilingExtension/TilingCodegen.py`:
- Around line 486-528: The docstring example for minimizeRectangle is incorrect:
update the example output at the first example (the one after ">>>
minimizeRectangle(rect, (4, 4))") so that the returned reference shape remains
(4, 4) (i.e. the tuple in the docstring should show (HyperRectangle(offset=(0,
0), dims=(2, 2)), (4, 4))). Locate the minimizeRectangle docstring and replace
the wrong output tuple with the corrected one, ensuring HyperRectangle, offset
and dims text matches the actual repr used elsewhere in the docstring.
🧹 Nitpick comments (2)
Deeploy/TilingExtension/TilingCodegen.py (1)
709-749: Preferzip(..., strict=True)after the length assert.This makes the intent explicit and keeps a length mismatch check even if asserts are optimized out; it also resolves Ruff B905. Please verify the project targets Python ≥ 3.10 before adopting.
Suggested change
- return sum(offset * stride for offset, stride in zip(offsets, strides)) + return sum(offset * stride for offset, stride in zip(offsets, strides, strict = True))Deeploy/CommonExtensions/OptimizationPasses/Matchers.py (1)
111-165: Add NotImplementedError to abstract methods for fail-fast behavior.The base class methods
_valid_patternand_nodes_map_from_anchorare currently silent no-ops. SinceSubgraphMatcheris never instantiated directly and both subclasses (NonBranchingMatcherandBranchingMatcher) properly override these methods with substantive implementations, raisingNotImplementedErroris safe and improves error handling if a new subclass accidentally fails to override them.♻️ Proposed fix
def _valid_pattern(self, pattern: gs.Graph) -> None: @@ - _ = pattern + raise NotImplementedError( + "SubgraphMatcher._valid_pattern must be implemented by subclasses" + ) @@ def _nodes_map_from_anchor(self, anchor: gs.Node, pattern: gs.Graph) -> Optional[Dict[str, gs.Node]]: @@ - _, _ = anchor, pattern + raise NotImplementedError( + "SubgraphMatcher._nodes_map_from_anchor must be implemented by subclasses" + )
Xeratec
force-pushed
the
pr/docs-and-debug
branch
from
ae4525a to
4dba05f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Deeploy/TilingExtension/TilingCodegen.py (1)
810-843:⚠️ Potential issue | 🟡 MinorDocstring claims "row-major order" but tile iteration is column-major.
Lines 815–816 state tiles are generated "in row-major order, iterating through dimensions from outermost to innermost," and line 832 repeats this. However,
nextTileIndex(lines 848–853) increments dimension 0 first and carries to higher indices, meaning the leftmost (outermost) index varies fastest — this is column-major (Fortran) order, not row-major (C) order where the rightmost index varies fastest.Proposed docstring fix
- The tiles are generated in row-major order, iterating through dimensions - from outermost to innermost. + The tiles are generated in column-major order, iterating through dimensions + from innermost to outermost (i.e., the first/leftmost index varies fastest).- Generate tile indices in row-major order. + Generate tile indices in column-major order.Deeploy/CommonExtensions/CodeTransformationPasses/MemoryAllocation.py (1)
172-215:⚠️ Potential issue | 🟠 MajortopologicallySortBuffers can re‑remove already‑sorted names.
When any references remain after the first pass, the next iteration tries to remove names that are already removed, which raises
ValueError. Skip buffers that are already sorted before removing.🩹 Proposed fix
while len(unsortedBufferNames) > 0: for buffer in buffers: + if buffer.name not in unsortedBufferNames: + continue if isinstance(buffer, _ReferenceBuffer) and buffer._referenceName in unsortedBufferNames: continue sortedBuffers.append(buffer) unsortedBufferNames.remove(buffer.name)
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 16-19: The changelog line "Move `AnnotateNeurekaWeightMemoryLevel`
to `Neureka` specific folder" needs hyphenation: update it to "Move
`AnnotateNeurekaWeightMemoryLevel` to `Neureka`-specific folder" so the term
Neureka-specific is correctly hyphenated; leave the other lines
(MemoryAwareClosureGeneration, MemoryAwarePrint*, sizeInBytes) unchanged.
In `@Deeploy/MemoryLevelExtension/CodeTransformationPasses/Closure.py`:
- Around line 114-121: The current condition incorrectly makes self.startRegion
ineffective; change the filtering so buffers are included when they actually
belong to the start region or fall within the intended start..end range: inside
the loop (makoDynamicReferences / ctxt.lookup -> buf._memoryLevel), replace the
redundant "buf._memoryLevel == self.startRegion or buf._memoryLevel !=
self.endRegion" with an explicit check such as "buf._memoryLevel ==
self.startRegion or (self.startRegion <= buf._memoryLevel < self.endRegion)" if
memory levels are orderable, or otherwise implement the correct membership test
for the start..end region; if startRegion is truly unused, remove it and
simplify to a single clear condition.
In `@Deeploy/Targets/Neureka/OptimizationPasses/MemoryLevelAnnotationPasses.py`:
- Around line 61-63: The condition using a strict less-than excludes buffers
that exactly fit remaining memory; in the allocation logic inside
MemoryLevelAnnotationPasses.py update the check in the block that references
weightMemoryOccupation, _neurekaWeightBufferSize(buffer), and
self._weightMemoryLevel.size to use <= instead of < so an exact-fit buffer is
allowed and still sets buffer._memoryLevel and updates weightMemoryOccupation.
🧹 Nitpick comments (5)
Deeploy/CommonExtensions/CodeTransformationPasses/IntrospectiveCodeTransformation.py (1)
180-191: Minor docstring style inconsistency:list of strvsList[str].Line 189 uses
list of strwhile every other docstring in this file usesList[str](e.g., lines 142, 219, 244, 280). Pick one convention — the rest of this file usesList[str].Proposed fix
- varNames : list of str - List of variable names to dereference within the template. + varNames : List[str] + The variable names to dereference within the template.Deeploy/CommonExtensions/OptimizationPasses/Matchers.py (1)
253-273: Missing blank line beforeNotessection header.All other docstrings in this file have a blank line separating section headers (e.g., before
Notes,Parameters,Returns). Line 263 is missing this separator, breaking the NumPy docstring convention and potentially causing doc-generation tools (e.g., Sphinx withnumpydoc) to misparse the section.📝 Proposed fix
The matching algorithm follows edges from the anchor node to build a complete mapping between pattern nodes and graph nodes, verifying operation types and attributes at each step. + Notes -----Deeploy/TilingExtension/TilingCodegen.py (1)
288-311:__add__docstring forTilingScheduleis accurate.One subtlety worth noting: the method uses
self'sinputBaseOffsets/outputBaseOffsetsfor the new schedule and silently discardsother's base offset values (only their keys are validated). The current docstring phrase "maintaining the same base offsets" is technically correct but a reader might not realizeother's offset values are dropped. Consider adding a brief note if this is intentional.Deeploy/Targets/Neureka/OptimizationPasses/MemoryLevelAnnotationPasses.py (1)
34-39:SequentialPassbase class is unnecessary here.
AnnotateNeurekaWeightMemoryLeveldefines its ownapplyand registers no sub-passes. Inheriting fromSequentialPassadds unused machinery. Consider inheriting fromPassdirectly.Deeploy/CommonExtensions/CodeTransformationPasses/PrintInputs.py (1)
397-401: Consider extracting duplicatedapplybodies into a shared helper.All three
applymethods (PrintInputGeneration,PrintOutputGeneration,PrintConstantGeneration) share the same structure: extract references → filter via_getRepDict→ calladdLeft/addRight. The only variation is the insertion side. A small base-class helper parameterized byaddLeftvsaddRightwould eliminate the repetition.
Deeploy/Targets/Neureka/OptimizationPasses/MemoryLevelAnnotationPasses.py
Show resolved
Hide resolved
Xeratec
force-pushed
the
pr/docs-and-debug
branch
from
4dba05f to
e9b8045
Compare
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 (3)
Deeploy/CommonExtensions/DataTypes.py (1)
12-97:⚠️ Potential issue | 🟡 MinorDocstrings for data type classes look good overall, but
float64_thas swapped mantissa/exponent.Line 93: The docstring reads "11-bit mantissa and 52-bit exponent", but the class attributes are
typeMantissa = 52andtypeExponent = 11. IEEE 754 double-precision has a 52-bit mantissa and 11-bit exponent, so the docstring has the values reversed.Proposed fix
class float64_t(FloatImmediate): - """64-bit float type with 11-bit mantissa and 52-bit exponent.""" + """64-bit float type with 52-bit mantissa and 11-bit exponent.""" typeName = "float64_t" typeWidth = 64 typeMantissa = 52 typeExponent = 11Deeploy/CommonExtensions/CodeTransformationPasses/MemoryAllocation.py (1)
199-215:⚠️ Potential issue | 🔴 CriticalBug:
topologicallySortBufferswill crash or produce duplicates on multi-pass iterations.The inner
forloop iterates over allbuffers(the original list) on every pass of thewhileloop. Buffers that were already sorted in a prior pass will be re-processed:sortedBuffers.append(buffer)creates a duplicate, andunsortedBufferNames.remove(buffer.name)raisesValueErrorbecause the name was already removed.This manifests whenever at least one
_ReferenceBufferis deferred to a later pass (i.e., any real dependency chain exists in the input).Proposed fix — skip already-sorted buffers
while len(unsortedBufferNames) > 0: for buffer in buffers: + if buffer.name not in unsortedBufferNames: + continue if isinstance(buffer, _ReferenceBuffer) and buffer._referenceName in unsortedBufferNames: continue sortedBuffers.append(buffer) unsortedBufferNames.remove(buffer.name)DeeployTest/testPrintInputOutputTransformation.py (1)
58-61:⚠️ Potential issue | 🔴 CriticalType mismatch:
MemoryLevelobject passed where astris expected.
defaultTargetMemoryLevelat lines 59-60 is aMemoryLevelobject, but_MemoryAwareGeneration.__init__(line 65 in Deeploy/MemoryLevelExtension/CodeTransformationPasses/PrintInputs.py) expectsmemoryHierarchyRegex: Optional[str]. This will raiseTypeErrorwhenre.compile(defaultTargetMemoryLevel)is called at line 77.Pass
defaultTargetMemoryLevel.nameinstead:Suggested fix
binding.codeTransformer.passes += [ - MemoryAwarePrintInputGeneration(defaultTargetMemoryLevel), - MemoryAwarePrintOutputGeneration(defaultTargetMemoryLevel), + MemoryAwarePrintInputGeneration(defaultTargetMemoryLevel.name), + MemoryAwarePrintOutputGeneration(defaultTargetMemoryLevel.name), ]
🤖 Fix all issues with AI agents
In `@Deeploy/CommonExtensions/OptimizationPasses/Matchers.py`:
- Around line 253-272: The NumPy-style docstring in the sequential pattern
matcher block is missing a blank line before the "Notes" section; update the
docstring in Deeploy/CommonExtensions/OptimizationPasses/Matchers.py (the
sequential matcher docstring that starts "Pattern matcher for sequential
computational graphs...") by inserting a single blank line between the end of
the preceding bullet list and the "Notes" header so the sections are separated
per NumPy docstring conventions.
🧹 Nitpick comments (4)
Deeploy/CommonExtensions/CodeTransformationPasses/MemoryAllocation.py (2)
260-260: Use idiomatic boolean checks instead of== False/== True.Flagged by Ruff (E712). Same applies to lines 348 and 365 in
MemoryPassthroughGeneration.Proposed fix for MemoryManagementGeneration
- assert buffer._live == False, f"Tried to allocate already live buffer {buffer.name}" + assert not buffer._live, f"Tried to allocate already live buffer {buffer.name}" ... - assert buffer._live == True, f"Tried to deallocate already dead buffer {buffer.name}" + assert buffer._live, f"Tried to deallocate already dead buffer {buffer.name}"Also applies to: 278-278
292-307: Docstring style inconsistency:Args:(Google-style) vs numpydoc used elsewhere.Line 304 uses
Args:while every other docstring in this file uses theParameters\n----------numpydoc convention.Proposed fix
- """Initialize the memory management passthrough pass. - - Args: - memoryHierarchyRegex (Optional[str], optional): A regex pattern to match memory hierarchy. - Defaults to None. - """ + """Initialize the memory management passthrough pass. + + Parameters + ---------- + memoryHierarchyRegex : Optional[str] + A regex pattern to match memory hierarchy. Defaults to None. + """Deeploy/MemoryLevelExtension/CodeTransformationPasses/PrintInputs.py (1)
143-193: The threeapplymethods are nearly identical — consider extracting a shared helper.
MemoryAwarePrintInputGeneration.apply,MemoryAwarePrintOutputGeneration.apply, andMemoryAwarePrintConstantGeneration.applyshare the same structure: extract references → filter by regex → get rep dict → add template. The only variation isaddLeftvsaddRight. A shared method in_MemoryAwareGenerationparameterized by side would eliminate this duplication.Also applies to: 217-267, 296-348
Deeploy/CommonExtensions/CodeTransformationPasses/PrintInputs.py (1)
153-156: Minor: inconsistent local variable naming acrossapplymethods.
PrintInputGeneration.applyusesrefDict(Line 154) whilePrintOutputGeneration.applyandPrintConstantGeneration.applyuserep(Lines 280, 398). Consider unifying for readability.Also applies to: 279-282, 397-400
| """ | ||
| Pattern matcher for sequential computational graphs without branching. | ||
|
|
||
| This matcher is optimized for patterns that form a simple chain of operations | ||
| without splits or merges in the computational flow. It uses a recursive | ||
| algorithm to follow the linear path of operations. | ||
|
|
||
| The matching algorithm follows edges from the anchor node to build a complete | ||
| mapping between pattern nodes and graph nodes, verifying operation types | ||
| and attributes at each step. | ||
| Notes | ||
| ----- | ||
| This matcher is efficient for linear operation sequences such as: | ||
| - Conv -> BatchNorm -> ReLU chains | ||
| - Linear -> Dropout -> Activation sequences | ||
| - Simple preprocessing pipelines | ||
|
|
||
| The algorithm assumes that each node in the pattern has at most one | ||
| output connection to the next node in the sequence. | ||
| """ |
There was a problem hiding this comment.
Missing blank line before the Notes section header.
NumPy-style docstrings require a blank line separating each section. There's a missing blank line between the end of the bullet list (line 268) and the Notes section header (line 263).
Proposed fix
- Simple preprocessing pipelines
The algorithm assumes that each node in the pattern has at most one
output connection to the next node in the sequence.
"""
- Notes
+
+ NotesWait — let me re-read. The issue is between line 262 and 263:
- Simple preprocessing pipelines
- Notes
+
+ NotesActually, to be precise:
- - Simple preprocessing pipelines
- Notes
+ - Simple preprocessing pipelines
+
+ Notes📝 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.
| """ | |
| Pattern matcher for sequential computational graphs without branching. | |
| This matcher is optimized for patterns that form a simple chain of operations | |
| without splits or merges in the computational flow. It uses a recursive | |
| algorithm to follow the linear path of operations. | |
| The matching algorithm follows edges from the anchor node to build a complete | |
| mapping between pattern nodes and graph nodes, verifying operation types | |
| and attributes at each step. | |
| Notes | |
| ----- | |
| This matcher is efficient for linear operation sequences such as: | |
| - Conv -> BatchNorm -> ReLU chains | |
| - Linear -> Dropout -> Activation sequences | |
| - Simple preprocessing pipelines | |
| The algorithm assumes that each node in the pattern has at most one | |
| output connection to the next node in the sequence. | |
| """ | |
| """ | |
| Pattern matcher for sequential computational graphs without branching. | |
| This matcher is optimized for patterns that form a simple chain of operations | |
| without splits or merges in the computational flow. It uses a recursive | |
| algorithm to follow the linear path of operations. | |
| The matching algorithm follows edges from the anchor node to build a complete | |
| mapping between pattern nodes and graph nodes, verifying operation types | |
| and attributes at each step. | |
| Notes | |
| ----- | |
| This matcher is efficient for linear operation sequences such as: | |
| - Conv -> BatchNorm -> ReLU chains | |
| - Linear -> Dropout -> Activation sequences | |
| - Simple preprocessing pipelines | |
| The algorithm assumes that each node in the pattern has at most one | |
| output connection to the next node in the sequence. | |
| """ |
🤖 Prompt for AI Agents
In `@Deeploy/CommonExtensions/OptimizationPasses/Matchers.py` around lines 253 -
272, The NumPy-style docstring in the sequential pattern matcher block is
missing a blank line before the "Notes" section; update the docstring in
Deeploy/CommonExtensions/OptimizationPasses/Matchers.py (the sequential matcher
docstring that starts "Pattern matcher for sequential computational graphs...")
by inserting a single blank line between the end of the preceding bullet list
and the "Notes" header so the sections are separated per NumPy docstring
conventions.
1 participant
This PR adds many missing docstring comments and improves debugging, especially when using a GUI debugger, by providing more helpful
__repr__()for the_ReferenceBufferclass. Additionally, it moves theMemoryAwareClosureGenerationandMemoryAwarePrint*passes from theCommonExtensionsto theMemoryLevelExtension.Added
__repr__()function for_ReferenceBuffercalssChanged
MemoryAwareClosureGenerationpass toMemoryLevelExtensionMemoryAwarePrint*passes toMemoryLevelExtensionsizeInBytesa class property instead of a functionAnnotateNeurekaWeightMemoryLeveltoNeurekaspecific folderPR Merge Checklist
develcommit and pointing todevel.CHANGELOG.mdfile has been updated.