idaes-pse icon indicating copy to clipboard operation
idaes-pse copied to clipboard

Keras file format updates

Open rundxdi opened this issue 10 months ago • 8 comments

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:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. 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 avatar Apr 25 '24 16:04 rundxdi

@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.

bpaul4 avatar Apr 25 '24 18:04 bpaul4

@rundxdi any progress on this?

ksbeattie avatar May 02 '24 18:05 ksbeattie

@rundxdi any progress on this?

Nothing this week -- still trying to figure out plotting tests.

rundxdi avatar May 04 '24 04:05 rundxdi

I have two remaining issues:

  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+.

  1. 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.

rundxdi avatar May 09 '24 01:05 rundxdi

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.

lbianchi-lbl avatar May 09 '24 19:05 lbianchi-lbl

As mentioned in the dev call, regarding the test failure in the power generation models you should:

  1. Mark the failing tests with pytest.mark.xfail
  2. 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.
  3. 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.

andrewlee94 avatar May 17 '24 14:05 andrewlee94

@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.

ksbeattie avatar May 23 '24 18:05 ksbeattie

@rundxdi, this missed the May release, now on the Aug release board.

ksbeattie avatar Jun 06 '24 18:06 ksbeattie

@rundxdi when you have a moment, could you please provide a status update? What is required to finalize this?

bpaul4 avatar Jul 09 '24 14:07 bpaul4

@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.

rundxdi avatar Jul 17 '24 01:07 rundxdi

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.

codecov-commenter avatar Jul 17 '24 02:07 codecov-commenter

Before merging this, we should remove Python 3.8 from the CI and see if the tests pass without needing to mark them as xfailing.

lbianchi-lbl avatar Jul 25 '24 18:07 lbianchi-lbl

@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.

bpaul4 avatar Aug 16 '24 14:08 bpaul4

@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 avatar Aug 16 '24 15:08 bpaul4

@bpaul4 I would say we need to get that examples PR merged first.

andrewlee94 avatar Aug 16 '24 15:08 andrewlee94

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

image

lbianchi-lbl avatar Aug 16 '24 19:08 lbianchi-lbl

@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.

andrewlee94 avatar Aug 16 '24 19:08 andrewlee94

@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:

image

lbianchi-lbl avatar Aug 16 '24 19:08 lbianchi-lbl