Skip to content

Do not skip validation between consecutive Elemwise inplace replacements #1494

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Jun 23, 2025

Closes #1420

There was a performance-related hack in the ElemwiseInplaceOptimizer where it tried to avoid validating the graph after replacing each node.

This is a bad idea because it may revert valid rewrites that happen to be caught in the same "check" window as an invalid rewrite. More importantly since we started inplacing on multi-output Elemwise, it could trigger an exception when trying to call has_destroyers on subsequent nodes, due to a previous invalid replacement.

It was hard to track down this issue because the special behavior was only triggered once a graph had more than 500 nodes.

After the refactor I noticed that most of the logic of the rewrite can be shared with Blockwise, so I went ahead and refactored it, which also closes #1457


📚 Documentation preview 📚: https://pytensor--1494.org.readthedocs.build/en/1494/

Copy link

codecov bot commented Jun 23, 2025

Codecov Report

Attention: Patch coverage is 92.85714% with 12 lines in your changes missing coverage. Please review.

Project coverage is 82.02%. Comparing base (236e50d) to head (1438953).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pytensor/tensor/rewriting/elemwise.py 92.66% 5 Missing and 3 partials ⚠️
pytensor/tensor/rewriting/blockwise.py 86.20% 2 Missing and 2 partials ⚠️

❌ Your patch check has failed because the patch coverage (92.85%) 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    #1494      +/-   ##
==========================================
+ Coverage   81.98%   82.02%   +0.04%     
==========================================
  Files         231      231              
  Lines       52192    52216      +24     
  Branches     9185     9186       +1     
==========================================
+ Hits        42790    42831      +41     
+ Misses       7094     7080      -14     
+ Partials     2308     2305       -3     
Files with missing lines Coverage Δ
pytensor/graph/destroyhandler.py 73.05% <100.00%> (+3.60%) ⬆️
pytensor/tensor/rewriting/blockwise.py 96.07% <86.20%> (-1.28%) ⬇️
pytensor/tensor/rewriting/elemwise.py 92.70% <92.66%> (+0.93%) ⬆️

... and 1 file 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 force-pushed the has_destroyers_bug branch 2 times, most recently from 65f2e35 to 6475c85 Compare June 24, 2025 00:11
@ricardoV94 ricardoV94 marked this pull request as ready for review June 24, 2025 09:05
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

This PR ensures validation after each in-place rewrite in the Elemwise optimizer, refactors shared logic for Blockwise in-place rewrites, and adds regression tests for both cases.

  • Enforce full graph validation between consecutive Elemwise in-place replacements
  • Refactor Blockwise in-place optimizer to extend the shared InplaceGraphOptimizer
  • Add tests for regression in Elemwise and partial in-place behavior in Blockwise

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.

File Description
tests/tensor/test_blockwise.py Add test_partial_inplace to cover Blockwise in-place behavior
tests/tensor/rewriting/test_elemwise.py Add test_InplaceElemwiseOptimizer_bug regression test for #1420
pytensor/tensor/rewriting/blockwise.py Refactor Blockwise in-place rewrite using InplaceGraphOptimizer
pytensor/graph/destroyhandler.py Introduce inplace_candidates helper for unified candidate logic
Comments suppressed due to low confidence (4)

tests/tensor/rewriting/test_elemwise.py:1535

  • [nitpick] Test function names should use snake_case. Consider renaming this to test_inplace_elemwise_optimizer_bug for consistency with pytest conventions.
def test_InplaceElemwiseOptimizer_bug():

tests/tensor/rewriting/test_elemwise.py:1544

  • The test references Elemwise but there is no corresponding import. Please add from pytensor.tensor.rewriting.elemwise import Elemwise at the top of the file.
    out1, out2 = Elemwise(ps.Composite([z1, z2], [z1 + z2, z2 - z1]))(z[1:], z[:-1])

pytensor/tensor/rewriting/blockwise.py:4

  • The imported vectorize_node is no longer used in this file. Consider removing this import to clean up unused code.
from pytensor.graph.replace import vectorize_node

pytensor/tensor/rewriting/blockwise.py:5

  • Neither copy_stack_trace nor out2in are used after the refactor. Removing these unused imports will improve code clarity.
from pytensor.graph.rewriting.basic import copy_stack_trace, out2in

@ricardoV94
Copy link
Member Author

ricardoV94 commented Jun 24, 2025

Actually I suspect with my second commit the inplace optimizer can't accidentally introduce invalid graphs anymore. If that's the case we could do a single validate at the end of the rewrite. I didn't actually try to see what's so slow about validate.

That's less priority than just the fix here

@ricardoV94 ricardoV94 force-pushed the has_destroyers_bug branch from 6475c85 to 1438953 Compare June 24, 2025 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
1 participant