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 to ensure older tensorflow versions still work #858

Merged
merged 1 commit into from
Oct 6, 2023
Merged

Fix to ensure older tensorflow versions still work #858

merged 1 commit into from
Oct 6, 2023

Conversation

ascillitoe
Copy link
Contributor

@ascillitoe ascillitoe commented Oct 5, 2023

Changing two type annotations for tf.keras.optimizers.legacy.Optimizer to forward references since tf.keras.optimizers.legacy doesn't exist in older (<2.9?) tensorflow versions. This resulted in the following error:

>>> from alibi_detect.saving import save_detector
AttributeError: module 'tensorflow.keras.optimizers' has no attribute 'legacy'

We could define new type aliases like OptimizersTF in alibi_detect/utils/_types.py, but it seems to be overly complicating things to me (we'd need three groups, one containing classes only, one types only, and one containing both...). We're also already using forwardref's for the tensorflow optimizers in other places...

P.s. have tested this with tensorflow == 2.8 and the error no longer occurs (when it does with master).

@ascillitoe ascillitoe added WIP PR is a Work in Progress DO NOT MERGE Do not merge this PR and removed WIP PR is a Work in Progress DO NOT MERGE Do not merge this PR labels Oct 5, 2023
@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #858 (2a986d8) into master (ce24f3c) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #858      +/-   ##
==========================================
- Coverage   81.98%   81.91%   -0.07%     
==========================================
  Files         159      159              
  Lines       10375    10375              
==========================================
- Hits         8506     8499       -7     
- Misses       1869     1876       +7     
Files Coverage Δ
alibi_detect/saving/_tensorflow/loading.py 85.20% <100.00%> (ø)
alibi_detect/saving/_tensorflow/saving.py 82.16% <100.00%> (ø)

... and 1 file with indirect coverage changes

@jklaise
Copy link
Contributor

jklaise commented Oct 6, 2023

Nice one!

@jklaise jklaise merged commit 9dbeaf3 into SeldonIO:master Oct 6, 2023
13 of 15 checks passed
@ascillitoe ascillitoe deleted the fix/old_tf_compat branch October 6, 2023 14:45
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.

2 participants