Skip to content

Conversation

Rocketknight1
Copy link
Member

@Rocketknight1 Rocketknight1 commented Jun 23, 2023

A sneaky bug was hiding in our Keras imports, where an import for call_context appeared to succeed on some TF versions, but actually got an older, unusable version of the function. This caused build() to behave improperly in some cases.

I went on a quest to fix this, and generally clean up our version-specific imports for TensorFlow to stop things like this from happening in future. I also bumped the minimum version for TF to 2.6 (2.6 should be 2 years old by the time of our next release), and eliminated the version cap in our dependency table because TF 2.13 should also be fully supported now.

This involved a partial rewrite of some code, where we checked for KerasTensor in a lot of places. However, this is an internal class for Keras and they keep moving it around, so trying to import it feels like a bad idea. Instead, I'm relying on tf.is_tensor(), which returns True for anything tensor-y, including symbolic tensors and Keras Input placeholders.

Fixes #24437

@Rocketknight1 Rocketknight1 requested review from gante and sgugger June 23, 2023 15:13
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 23, 2023

The documentation is not available anymore as the PR was closed or merged.

@ydshieh
Copy link
Collaborator

ydshieh commented Jun 23, 2023

Keras and they keep moving it around

It's tensor-flow ... that's probably the reason.

@Rocketknight1
Copy link
Member Author

Rocketknight1 commented Jun 23, 2023

I tested this with a spread of TF versions going back to 2.6. Even at 2.6, it was pretty hard to get an environment that worked with modern transformers - old TensorFlow keeps trying to use NumPy features that have been deprecated and deleted, but our modern libraries need a minimum version of NumPy to run at all, so there's actually only a very narrow window of NumPy versions that can even run both at once! I think going back to 2.5 or earlier would be very difficult, so I'm pretty comfortable with bumping our minimum version at this point.

In all versions I tested with this patch, our test suite runs well and the issue identified in #24437 is fixed, so this should be ready to go after it's reviewed!

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup. I will agree to not put a maximum pin on TensorFlow when they stop making breaking changes in minor releases (so never 😅 )

setup.py Outdated
"tensorflow-cpu>=2.4,<2.13",
"tensorflow>=2.4,<2.13",
"tensorflow-text<2.13",
"tensorflow-cpu>=2.6",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No let's leave a maximum pin. I do not want any surprises.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deal, but I'm bumping the cap to <2.14, since 2.13 works fine!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

@Rocketknight1 Rocketknight1 merged commit 8e164c5 into main Jun 23, 2023
@Rocketknight1 Rocketknight1 deleted the improved_keras_imports branch June 23, 2023 18:09
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.

TFPreTrainedModel.build breaks pytorch PreTrainedModel.from_pretrained(from_tf=True)
4 participants