-
Notifications
You must be signed in to change notification settings - Fork 134
Add Feature that can go back and forward in rewrite history #874
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
81933fc
to
a7a116f
Compare
a7a116f
to
dfcecd6
Compare
dfcecd6
to
84e56ce
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (35.93%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #874 +/- ##
==========================================
- Coverage 82.12% 82.03% -0.09%
==========================================
Files 211 214 +3
Lines 49757 50398 +641
Branches 8819 8897 +78
==========================================
+ Hits 40862 41345 +483
- Misses 6715 6848 +133
- Partials 2180 2205 +25
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds a new FullHistory
feature to record and navigate rewrite steps on a FunctionGraph
, along with a corresponding test and an IPython widget for interactive exploration.
- Implements
FullHistory
inpytensor/graph/features.py
to capture and rewind/fast-forward graph rewrites - Adds
test_full_history
intests/graph/test_features.py
and updates imports for rewrite testing - Introduces
InteractiveRewrite
widget inpytensor/ipython.py
, and adjusts test/CI configs to skip it
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
pytensor/graph/features.py | New FullHistory feature for tracking and replaying rewrites |
tests/graph/test_features.py | Updated imports and added test_full_history |
pytensor/ipython.py | New interactive widget (InteractiveRewrite ) for notebook use |
pyproject.toml | Excludes pytensor/ipython.py from pytest runs |
.github/workflows/test.yml | Updates CI to ignore pytensor/ipython.py in doctests |
Comments suppressed due to low confidence (4)
pytensor/graph/features.py:536
- [nitpick] The internal list name 'fw' is ambiguous; consider renaming it to something more descriptive like 'forward_steps' or 'forward_history' for better readability.
self.fw = []
pytensor/graph/features.py:554
- [nitpick] Add a detailed docstring to
goto
explaining the valid range ofcheckpoint
, how it maps to history states, and its effect onpointer
.
def goto(self, checkpoint):
pytensor/ipython.py:1
- The new
InteractiveRewrite
widget isn’t covered by any tests; consider adding a basic smoke test (e.g., instantiating the widget and verifying core attributes) to prevent regressions.
import anywidget
pytensor/ipython.py:6
- Verify that
Variable
(andrewrite_graph
) are exported frompytensor.graph
; if not, importVariable
frompytensor.graph.basic
andrewrite_graph
frompytensor.graph.rewriting.utils
to avoid import errors.
from pytensor.graph import FunctionGraph, Variable, rewrite_graph
""" | ||
history_len = len(self.bw) | ||
pointer = self.pointer | ||
assert 0 <= checkpoint <= history_len |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using assert
for input validation can be bypassed in optimized runs; consider raising a ValueError
for out-of-range checkpoint
values instead.
assert 0 <= checkpoint <= history_len | |
if not (0 <= checkpoint <= history_len): | |
raise ValueError(f"Checkpoint value {checkpoint} is out of range. It must be between 0 and {history_len}.") |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall. It does feel like it should have a few more unit tests, but unsure concretely which ones to add.
content = traitlets.Unicode("").tag(sync=True) | ||
|
||
_esm = """ | ||
function render({ model, el }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these should be put in a widget.js
and a widget.css
file instead of hanging out as a giant string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I would keep it together, until and if it actually grows larger. It's a single file this way instead of a folder. We can always refactor later
Description
I can imagine this being very powerful for debugging or reasoning about rewrites?
Screencast.From.2025-06-10.19-03-08.mp4
Related Issue
Checklist
Type of change