Skip to content

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

Merged
merged 2 commits into from
Jun 11, 2025

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Jun 28, 2024

Description

I can imagine this being very powerful for debugging or reasoning about rewrites?

Screencast.From.2025-06-10.19-03-08.mp4
import pytensor
import pytensor.tensor as pt
from pytensor.graph.fg import FunctionGraph
from pytensor.graph.features import FullHistory
from pytensor.graph.rewriting.utils import rewrite_graph

x = pt.scalar("x")
out = pt.log(pt.exp(x) / pt.sum(pt.exp(x)))

fg = FunctionGraph(outputs=[out])
history = FullHistory()
fg.attach_feature(history)

rewrite_graph(fg, clone=False, include=("canonicalize", "stabilize"))

# Replay rewrites
history.start()
pytensor.dprint(fg)
pytensor.config.optimizer_verbose = True
for i in range(3):    
    print()
    print(">>> ", end="")
    pytensor.dprint(history.next())
        
# Log [id A] 4
#  └─ True_div [id B] 3
#     ├─ Exp [id C] 2
#     │  └─ x [id D]
#     └─ Sum{axes=None} [id E] 1
#        └─ Exp [id F] 0
#           └─ x [id D]
# >>> MergeOptimizer
# Log [id A] 3
#  └─ True_div [id B] 2
#     ├─ Exp [id C] 0
#     │  └─ x [id D]
#     └─ Sum{axes=None} [id E] 1
#        └─ Exp [id C] 0
#           └─ ···
# >>> local_mul_canonizer
# Log [id A] 1
#  └─ Softmax{axis=None} [id B] 0
#     └─ x [id C]
# >>> local_logsoftmax
# LogSoftmax{axis=None} [id A] 0
#  └─ x [id B]

# Or in reverse
for i in range(3):
    print()
    print(">>> ", end="")
    pytensor.dprint(history.prev())

# >>> local_logsoftmax
# Log [id A] 1
#  └─ Softmax{axis=None} [id B] 0
#     └─ x [id C]
# >>> local_mul_canonizer
# Log [id A] 3
#  └─ True_div [id B] 2
#     ├─ Exp [id C] 0
#     │  └─ x [id D]
#     └─ Sum{axes=None} [id E] 1
#        └─ Exp [id C] 0
#           └─ ···
# >>> MergeOptimizer
# Log [id A] 4
#  └─ True_div [id B] 3
#     ├─ Exp [id C] 2
#     │  └─ x [id D]
#     └─ Sum{axes=None} [id E] 1
#        └─ Exp [id F] 0
#           └─ x [id D]
    
pytensor.config.optimizer_verbose = False
# Or go to any step
pytensor.dprint(history.goto(2))
# Log [id A] 1
#  └─ Softmax{axis=None} [id B] 0
#     └─ x [id C]

Related Issue

  • Closes #
  • Related to #

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

@ricardoV94 ricardoV94 added enhancement New feature or request graph rewriting labels Jun 28, 2024
@ricardoV94 ricardoV94 marked this pull request as draft July 5, 2024 19:33
@ricardoV94 ricardoV94 force-pushed the full_rewrite_history branch 2 times, most recently from 81933fc to a7a116f Compare June 10, 2025 17:04
@pymc-devs pymc-devs deleted a comment from review-notebook-app bot Jun 10, 2025
@ricardoV94 ricardoV94 marked this pull request as ready for review June 10, 2025 17:05
@ricardoV94 ricardoV94 force-pushed the full_rewrite_history branch from a7a116f to dfcecd6 Compare June 10, 2025 17:09
@ricardoV94 ricardoV94 force-pushed the full_rewrite_history branch from dfcecd6 to 84e56ce Compare June 10, 2025 17:43
Copy link

codecov bot commented Jun 10, 2025

Codecov Report

Attention: Patch coverage is 35.93750% with 82 lines in your changes missing coverage. Please review.

Project coverage is 82.03%. Comparing base (d10f245) to head (84e56ce).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
pytensor/ipython.py 0.00% 78 Missing ⚠️
pytensor/graph/features.py 92.00% 2 Missing and 2 partials ⚠️

❌ 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

Impacted file tree graph

@@            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     
Files with missing lines Coverage Δ
pytensor/graph/features.py 67.63% <92.00%> (+3.06%) ⬆️
pytensor/ipython.py 0.00% <0.00%> (ø)

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ricardoV94 ricardoV94 requested review from zaxtax and Copilot June 10, 2025 20:03
Copy link

@Copilot Copilot AI left a 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 in pytensor/graph/features.py to capture and rewind/fast-forward graph rewrites
  • Adds test_full_history in tests/graph/test_features.py and updates imports for rewrite testing
  • Introduces InteractiveRewrite widget in pytensor/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 of checkpoint, how it maps to history states, and its effect on pointer.
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 (and rewrite_graph) are exported from pytensor.graph; if not, import Variable from pytensor.graph.basic and rewrite_graph from pytensor.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
Copy link
Preview

Copilot AI Jun 10, 2025

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.

Suggested change
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.

Copy link
Contributor

@zaxtax zaxtax left a 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 }) {
Copy link
Contributor

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.

Copy link
Member Author

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

@ricardoV94 ricardoV94 merged commit 5f5be92 into pymc-devs:main Jun 11, 2025
72 of 73 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request graph rewriting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants