-
Notifications
You must be signed in to change notification settings - Fork 147
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 Report❌ Patch coverage is
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
FullHistoryinpytensor/graph/features.pyto capture and rewind/fast-forward graph rewrites - Adds
test_full_historyintests/graph/test_features.pyand updates imports for rewrite testing - Introduces
InteractiveRewritewidget 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
gotoexplaining the valid range ofcheckpoint, how it maps to history states, and its effect onpointer.
def goto(self, checkpoint):
pytensor/ipython.py:1
- The new
InteractiveRewritewidget 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, importVariablefrompytensor.graph.basicandrewrite_graphfrompytensor.graph.rewriting.utilsto avoid import errors.
from pytensor.graph import FunctionGraph, Variable, rewrite_graph
| """ | ||
| history_len = len(self.bw) | ||
| pointer = self.pointer | ||
| assert 0 <= checkpoint <= history_len |
Copilot
AI
Jun 10, 2025
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}.") |
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