-
Notifications
You must be signed in to change notification settings - Fork 117
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
Hacky fix for dictionary output with tf 2.14 #933
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main keras-team/keras-core#933 +/- ##
==========================================
+ Coverage 76.82% 83.83% +7.00%
==========================================
Files 329 318 -11
Lines 31427 28743 -2684
Branches 6112 5491 -621
==========================================
- Hits 24144 24096 -48
+ Misses 5719 3141 -2578
+ Partials 1564 1506 -58
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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.
Thanks for the PR!
@@ -46,10 +47,20 @@ class Function(Operation): | |||
def __init__(self, inputs, outputs, name=None): | |||
super().__init__(name=name) | |||
|
|||
if backend() == "tensorflow": |
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.
Can we do this in keras_core/backend/tensorflow/layer.py::TFLayer.__init__
instead?
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 don't think so, because we can't turn off tf auto tracking overall for functional models. We only want to turn it off for the input/output structs here to avoid the tf bug. Essentially we need a way to run a hook inside Function.__init__
.
The nice way to avoid all this would be to use Tracker
for our tf tracking. This could save a lot of bugs I think, but is more of an effort. https://github.com/keras-team/keras-core/issues/934#issuecomment-1728464445
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.
Maybe we could just not depend on AutoTrackable
(just Trackable
) and then manually track everything according to our own Tracker
.
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.
Yeah, I think this would take some work, but really improve the overall state if we did.
4a040bb
to
55e9c46
Compare
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
@@ -46,10 +47,20 @@ class Function(Operation): | |||
def __init__(self, inputs, outputs, name=None): | |||
super().__init__(name=name) | |||
|
|||
if backend() == "tensorflow": |
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.
Maybe we could just not depend on AutoTrackable
(just Trackable
) and then manually track everything according to our own Tracker
.
See keras-team/keras#18399 for more info. And other workaround we could consider.