[To keep] -- [build] tf upgrade by keeping keras v2

[felixdittrich92]

This PR:

• upgrade TF by staying at keras v2

@odulcy-mindee This would work if we want to upgrade tf by keeping keras at version 2 without the need to change the current model saving/loading / refactoring everything (would be only a temp fix)

I'm super unhappy with this PR but i don't see an alternative atm 😓

[odulcy-mindee]

Why did you set TF_USE_LEGACY_KERAS=1 everywhere as it is the default behavior of your PR ?

[felixdittrich92]

Why did you set TF_USE_LEGACY_KERAS=1 everywhere as it is the default behavior of your PR ?

The behavior of this env variable is super strange:

By installing TF>= 2.16 it installs automatically keras>=3.0.0 so tf.keras is additonal installed (to keep keras v2 backwards compatibility with tf >= 2.16)

I'm not sure why it works for everything ..without the latency scripts there it jumps all the time to keras 3 (also on the CI job) without setting TF_USE_LEGACY_KERAS=1 in front of your script - which leads to errors

[felixdittrich92]

@odulcy-mindee Now it works

• Added as function instead of os.environ .. in every script which makes it easier to remove later on if we have a solution with keras v3

[codecov]

Codecov Report

Attention: Patch coverage is 94.28571% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.46%. Comparing base (dccc26b) to head (9940348).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
doctr/file_utils.py	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1542      +/-   ##
==========================================
+ Coverage   96.39%   96.46%   +0.07%     
==========================================
  Files         164      164              
  Lines        7823     7869      +46     
==========================================
+ Hits         7541     7591      +50     
+ Misses        282      278       -4     

Flag	Coverage Δ
unittests	96.46% <94.28%> (+0.07%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[odulcy-mindee]

Why no more tf.function ?

[felixT2K]

Before it was disabled globally so no tf.function has had any effect
the functions where the tf.function annotation is removed makes use of numpy which is not compatible with the compilation (which is triggered for the tf.function annotated functions)

[felixdittrich92]

On hold

[felixdittrich92]

@odulcy-mindee Our fallback if we don't have a working version with keras v3 end of the year :)

[felixdittrich92]

@odulcy-mindee After digging (again) a bit deeper into the keras v3 upgrade:

• Mostly every model would require a re-training (sar_resnet31 complete broken - keras v3 LSTMCell bug)
• Regression bug for LSTM's which makes it not possible to load from the keras v2 saved weights
• Most of our models have to run in eager mode but keras v3 ignores tf.config.run_functions_eagerly(True) completly on GPU 😅
• By default keras v3 uses jit compilation on GPU anyway disabling this takes no effect (on CPU all models expect sar_resnet31 can be trained and loaded afterwards without trouble - on GPU only a few models because jit raises issues)

So i would suggest to stick with the solution of the PR ftm. wdyt ?

[felixdittrich92]

@odulcy-mindee
The breaking point here is that doctr needs to be imported before tensorflow:

two options:

• we keep the ensure_keras_v2 to make it explicit
• we import doctr (in the training / eval scripts) first and ignore ruff's (isort)

What would you prefer ?

[odulcy-mindee]

Keep ensure_keras_v2 function, because option 2 will be a hell to maintain in my opinion

[felixdittrich92]

Yes i agree :D
@odulcy-mindee btw could you check your slack ? ^^

[odulcy-mindee]

@felixdittrich92

1. When you write "keras v3 LSTMCell bug", do you mean is it a known bug or does it have a different behavior than before ?
2. About the jit error, is it related to what has been written here ? Keras 2 <> Keras 3 incompatibilities keras-team/keras#18467

Keras 3 has jit_compile=True by default -- this might not work with all TF ops, so with some custom models/layers you might have set jit_compile=False if you see an XLA related error.

If you don't have any solution, let's keep the PR likes that

[felixdittrich92]

@felixdittrich92

1. When you write "keras v3 LSTMCell bug", do you mean is it a known bug or does it have a different behavior than before ?
2. About the jit error, is it related to what has been written here ? Keras 2 <> Keras 3 incompatibilities keras-team/keras#18467

Keras 3 has jit_compile=True by default -- this might not work with all TF ops, so with some custom models/layers you might have set jit_compile=False if you see an XLA related error.

If you don't have any solution, let's keep the PR likes that

1.It's a bug but i need to open a ticket for this (not done yet) get_initial_state is available and takes the same args but if you pass anything it raises an error
2. Correct :) but disabling on the keras.Model with self.jit_compile = False does exactly nothing 😓

[felixdittrich92]

@odulcy-mindee
Then i would say ready to review 😅

[felixdittrich92]

We will move to keras v3 at any time but it's really hard and i spent already a lot (to much 😅) time on it
But i will come up with some PR's which prepares further for v3 but will work also with v2

[felixdittrich92]

Closes: #1673

[odulcy-mindee]

Ok, let's keep it like that, thank you !