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

Try to fix sphinx problem by restricting tensorflow-model-optimization #967

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

jmitrevs
Copy link
Contributor

@jmitrevs jmitrevs commented Feb 9, 2024

Description

The sphinx build failed. This tries to restrict incompatible versions

  • Bug fix (non-breaking change that fixes an issue)

Tests

Check the sphinx build

Checklist

  • I have read the guidelines for contributing.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have installed and run pre-commit on the files I edited or added.
  • I have added tests that prove my fix is effective or that my feature works.

@jmitrevs jmitrevs added bug please test Trigger testing by creating local PR branch labels Feb 9, 2024
@jmitrevs
Copy link
Contributor Author

jmitrevs commented Feb 9, 2024

@jmduarte, @vloncar, is this the type of fix you think is appropriate for sphinx? This is actually for everyone that install hls4ml.

@vloncar
Copy link
Contributor

vloncar commented Feb 12, 2024

After some more digging, this is due to the split from keras and TF and the way this is handled in TF 2.15 onwards. Apparently, TFMOT works only with keras 2.x from TF (tf.keras) and after the split this is provided by the tf_keras package. TFMOT added this to requrements.txt but not to setup.py so it doesn't get installed on fresh environments. I opened tensorflow/model-optimization#1117 to track this. I propose we wait for the resolution of that one in a timely manner, and if not, we go with this workaround.

@jmitrevs
Copy link
Contributor Author

Do we need tf_keras in all cases? Maybe we should add it to our requirements.

If we add tf_keras then we don't need to restrict tensorflow-model-optimization, right?

@jmitrevs jmitrevs marked this pull request as draft February 12, 2024 22:15
@jmitrevs
Copy link
Contributor Author

I am going to try the alternate solution to see if it works.

@vloncar
Copy link
Contributor

vloncar commented Feb 12, 2024

tf_keras only exists for 2.14.1 and 2.15. Not sure if this will force the tensorflow to that version or if not, will it affect previous tensorflow versions. I also didn't figure out what would happen if I install keras 3. This situation is a bit of a mess 🙂

@jmitrevs
Copy link
Contributor Author

The alternate version may be more problematic based on your comments, so I won't push it. (I do think it works in my setup, though.) So we wait to see if it gets solved upstream first?

@jmitrevs jmitrevs marked this pull request as ready for review February 14, 2024 20:38
@jmitrevs
Copy link
Contributor Author

Ideally this is a temporary fix that will be reverted.

@jmitrevs jmitrevs merged commit 77e746e into fastmachinelearning:main Feb 14, 2024
8 checks passed
@jmitrevs jmitrevs deleted the tf_sphinx_fix branch February 14, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug please test Trigger testing by creating local PR branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants