idaes-pse
idaes-pse copied to clipboard
Keras file format updates
Fixes
Addresses file format changes associated with updates to tensorflow and keras mentioned in issue #1369
Summary/Motivation:
Tensorflow and keras version updates changed how they save/load files. This PR updates the keras_surrogate.py file to accommodate those changes.
Changes proposed in this PR:
- Update keras_surrogate.py to save new .keras file format
- Update test_keras_surrogate.py with new versions of saved models and accounting for new save/load methods
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution:
- I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
- I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.
@rundxdi thank you for opening this. Once this is ready to test, it would be good to undo the temporary fix implemented in https://github.com/IDAES/idaes-pse/pull/1373 to ensure the latest version of TensorFlow is used in the tests.
@rundxdi any progress on this?
@rundxdi any progress on this?
Nothing this week -- still trying to figure out plotting tests.
I have two remaining issues:
- Python 3.8 doesn't like test_keras_surrogate.py. Tests all produce failure statuses looking like:
Exception encountered: Unrecognized keyword arguments: ['batch_shape']
These tests all pass on Python 3.9+.
- The test_sofc_surrogates.py tests rely on a saved Keras surrogate that cannot be loaded with Keras v3+ (and thus cannot be loaded with Tensorflow 2.16.1). I've found no obvious way to generate an equivalent surrogate in Keras v3 equivalent surrogate; their suggested workaround doesn't work.
1. Python 3.8 doesn't like test_keras_surrogate.py. Tests all produce failure statuses looking like:
Exception encountered: Unrecognized keyword arguments: ['batch_shape']
These tests all pass on Python 3.9+.
My guess is that this is due to NumPy (and consequently I imagine most of the downstream projects depending on it) having dropped support for Python 3.8 a while back. So, when installing on Python 3.8, an older version of the package (the latest compatible) gets installed, which I assume doesn't support the same arguments as the current version.
We're considering removing support for (i.e. stopping testing with) Python 3.8. In the meantime, you should be able to use pytest.mark.skipif()
to skip the failing tests if they're being run: https://docs.pytest.org/en/latest/how-to/skipping.html#id1
2. The test_sofc_surrogates.py tests rely on a saved Keras surrogate that cannot be loaded with Keras v3+ (and thus cannot be loaded with Tensorflow 2.16.1). I've found no obvious way to generate an equivalent surrogate in Keras v3 equivalent surrogate; their suggested workaround doesn't work.
With the disclaimer that I don't have any significant hands-on experience using Keras, I'm at least aware of there being limited or no support for cross-version compatibility (i.e. a model serialized with Keras version X won't work when you try to deserialize it using version Y). I'm not sure how much the new (current) serialization format changes things. If this is the case, I guess the solution would be to regenerate the file used in the tests using the current version. I'm not sure if this addresses the original issue, though.
As mentioned in the dev call, regarding the test failure in the power generation models you should:
- Mark the failing tests with
pytest.mark.xfail
- In the failing code, add a deprecation warning indicating the tests for this code have started failing and the code will be removed no earlier than August if it is not fixed.
- Open an issue to fix this and assign it to the code owner(s), and put this on the August release board as a High priority so we track it.
@rundxdi we expect to cut the May release next week, so if this PR is to be included, it needs to be merged very soon.
@rundxdi, this missed the May release, now on the Aug release board.
@rundxdi when you have a moment, could you please provide a status update? What is required to finalize this?
@rundxdi when you have a moment, could you please provide a status update? What is required to finalize this?
Hi @bpaul4 -- we can safely remove any of the non .keras
files. The Python 3.8 tests should be skipped at this point.
It should be ready to finalize/review for finalization.
Codecov Report
Attention: Patch coverage is 73.33333%
with 4 lines
in your changes missing coverage. Please review.
Project coverage is 76.36%. Comparing base (
c2825ca
) to head (0c2775e
). Report is 1 commits behind head on main.
Files | Patch % | Lines |
---|---|---|
idaes/core/surrogate/keras_surrogate.py | 77.77% | 2 Missing :warning: |
...generation/properties/sofc/sofc_keras_surrogate.py | 66.66% | 2 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #1401 +/- ##
==========================================
- Coverage 76.38% 76.36% -0.03%
==========================================
Files 394 394
Lines 65121 65126 +5
Branches 14427 14426 -1
==========================================
- Hits 49745 49732 -13
- Misses 12813 12833 +20
+ Partials 2563 2561 -2
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Before merging this, we should remove Python 3.8 from the CI and see if the tests pass without needing to mark them as xfail
ing.
@andrewlee94 it looks like all of your prior review comments have been addressed.
I removed the xfail markers from the keras surrogate tests and everything looks good.
@andrewlee94 how would you suggest handling the failing example integration tests? The failures are resolved as part of https://github.com/IDAES/examples/pull/128.
@bpaul4 I would say we need to get that examples PR merged first.
There might be another test that needs to be skipped/xfailed as it's currently causing a failure in the "user install" integration checks: https://github.com/IDAES/idaes-pse/actions/runs/10424410674/job/28873157657?pr=1401#step:6:538
@lbianchi-lbl I think we should look into that one a bit more first, and fix it if we can. Being that is in the core code, we should at least know why it fails.
@lbianchi-lbl I think we should look into that one a bit more first, and fix it if we can. Being that is in the core code, we should at least know why it fails.
The fact that it's only failing in the user-mode check, plus the exception raised, seem to point towards the file not being included in the non-developer installation: