Allow overwriting existing rewrites during registration #1119
Conversation
|
I want to add tests but am unsure of the best approach. Will a simple test like the one below suffice?
|
Better to create a separate database just for testing, and register directly to it (no decorator). It should fail on the second attempt, but pass on the third when pass an |
|
@ricardoV94 done. Thanks. |
|
@ricardoV94 please let me know the further steps. Thanks!! |
pytensor/graph/rewriting/db.py
Outdated
| if overwrite_existing: | ||
| raise ValueError( | ||
| f"The tag '{name}' does not exist in the database. Cannot be overwritten." | ||
| ) |
There was a problem hiding this comment.
This isn't needed. It's enough not to fail when it already exists
pytensor/graph/rewriting/db.py
Outdated
| if not overwrite_existing: | ||
| self.add_tags(name, *tags) |
There was a problem hiding this comment.
Can we remove the tags in the previous block, so this doesn't need to be conditional?
tests/graph/rewriting/test_db.py
Outdated
| db.register("c", NewTestRewriter()) # name taken | ||
|
|
||
| db.register("c", NewTestRewriter(), overwrite_existing=True) |
There was a problem hiding this comment.
Can we use a separate test for readability?
I would like to test the new rewrite actually replaces the old one. You can make two rewrites that do nothing but update a global counter and check the first time you rewrite an fgraph the first counter goes up (but not the second) and vice-versa
There was a problem hiding this comment.
Still check the raises in the first time you try to redefine the rewrite (and that the original rewrite isn't broken by this attempt)
There was a problem hiding this comment.
@ricardoV94 I implemented the additonal test as you mentioned. Please let me know if it matches with the idea you had in mind.
and include appropriate testing
AdvH039
force-pushed
the
add_overwrite_existing_flag
branch
from
aac4457 to
aefb963
Compare
|
@ricardoV94 please let me know about further steps to be taken. Thank you !! |
|
Hi @AdvH039 I wanted to use the DB user-facing API on the tests, but I didn't know it by heart so I checked the PR and changed it myself. I also put the helper classes inside the relevant test function to keep things a bit tidier. Anyway the logic seems to be working (it already was). Let me know if the changes I pushed look good and we can merge this one. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1119 +/- ##
==========================================
+ Coverage 82.27% 82.29% +0.01%
==========================================
Files 186 186
Lines 48000 48014 +14
Branches 8621 8625 +4
==========================================
+ Hits 39490 39511 +21
+ Misses 6353 6346 -7
Partials 2157 2157
|
|
@ricardoV94 the changes look great. We can merge it. |
|
@AdvH039 this was great, looking forward to your next PR if you're so inclined! |
|
@ricardoV94 Do you know any compiler related issues here that are beginner/intermediate? |
|
@AdvH039 I'm sure we do, but may not have opened them. What sort of compiler-related stuff interests you? |
|
@ricardoV94 anything really. I just want the experience. But it would be nice if it had something to do with compiler optimization. |
ricardoV94
changed the title
Description
Related Issue
Checklist
Type of change
📚 Documentation preview 📚: https://pytensor--1119.org.readthedocs.build/en/1119/