Replace str "output" by a dummy Op in the clients of the FunctionGraph #790
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ricardoV94
force-pushed
the
remove_output_client
branch
5 times, most recently
from
ba93370 to
a9204fb
Compare
ricardoV94
force-pushed
the
remove_output_client
branch
2 times, most recently
from
4b9ed3a to
3ab8992
Compare
ricardoV94
force-pushed
the
remove_output_client
branch
3 times, most recently
from
707ce87 to
f7da400
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #790 +/- ##
==========================================
+ Coverage 80.76% 80.78% +0.01%
==========================================
Files 163 163
Lines 46938 46921 -17
Branches 11497 11490 -7
==========================================
- Hits 37911 37903 -8
+ Misses 6785 6774 -11
- Partials 2242 2244 +2
|
jessegrabowski
approved these changes
Jul 6, 2024
| ApplyOrOutput = Apply | Literal["output"] | ||
| ClientType = tuple[ApplyOrOutput, int] | ||
|
|
||
| class Output(Op): |
There was a problem hiding this comment.
It's quite surprising we don't have another class named Output anywhere else in the codebase
|
Marking as a draft because I want to do a release before these changes |
ricardoV94
force-pushed
the
remove_output_client
branch
from
f7da400 to
5b8f38a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is often a source of bug, because everything in the clients graph is a node with an Op, except these sneaky string labels "output". It also messes up mypy in multiple places.
The whole point of having "output" in the clients dictionary is so we know if a variable is used in multiple places (i.e., may still need to be computed even if we do a rewrite that gets rid of it as an intermediate operation). For that purpose it doesn't really matter if it's an output or used by another node. This approach allows handling these cases more easily because it looks like just another client.
We could also get rid of this dummy client, and force rewrites to check if a node is in fgraph.outputs separately but that feels more cumbersome. I understand why they added "output" as a client. I just think it hurts a bit more than needed to have it be so different.
Having said and done this PR I'm happy to just close it if people don't agree with it.
Related Issue
Checklist
Type of change