Skip to content
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

fix keras compatibility bug in factorized_top_k #717

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

getumen
Copy link

@getumen getumen commented May 17, 2024

The bug is summarized by the following issue comment.
#712 (comment)

Copy link

google-cla bot commented May 17, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@mat10599
Copy link

@maciejkula

@ghost
Copy link

ghost commented Jun 21, 2024

+1 for this PR getting merged in. It was a blocker for me until I manually applied this fix.

@takoyaro
Copy link

takoyaro commented Jul 9, 2024

Can we please get this merged?

@rmminusrslash
Copy link

rmminusrslash commented Jul 9, 2024

I have the same issue. most active contributor seems to be @maciejkula, can you please tell us how to get this merged and released?

Edit: This change is not sufficient to fix the bug. We also need to change how the counter is accessed, this part in the Streamingclass throws an exception

      """Enumerates rows in each batch using a total element counter."""

      starting_counter = self._counter.read_value()

Who is running CI/CD for the tutorial code? https://www.tensorflow.org/recommenders/examples/quickstart
It throws an exception due to the bug in this PR, so it appears the tensorflow recommender library might not be maintained if even the quickstart does not work

@czroth
Copy link

czroth commented Jul 12, 2024

@ZhaoyueCheng, it looks like you are the only active commiter to main in the last 6 mo. Can you give us a hand?

@ZhaoyueCheng
Copy link
Contributor

ZhaoyueCheng commented Jul 22, 2024

I don't have permission to merge PRs actually, but since this repository is mostly running with legacy keras, would using TF_USE_LEGACY_KERAS=1 work as a workaround with older keras version? (also suggested in #712 (comment))

@Az-r-ow
Copy link

Az-r-ow commented Jul 31, 2024

I don't have permission to merge PRs actually, but since this repository is mostly running with legacy keras, would using TF_USE_LEGACY_KERAS=1 work as a workaround with older keras version? (also suggested in #712 (comment))

Passing TF_USE_LEGACY_KERAS=1 is not doing it for me, using the legacy version of keras is making my notebook kernel crash without any logs :

[error] Disposing session as kernel process died ExitCode: undefined, Reason: 

It should be related to the fact that I’m using some of keras latest features, however at this stage of my project it’s frustrating to go back and adjust the code in question.

@y-71
Copy link

y-71 commented Aug 4, 2024

@rmminusrslash that issue is independant, it has been reported at #722 and there is this patch #723 linked to it

@keivanipchihagh
Copy link

+1
I applied this change and fixed the issue described in #712.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants