search

IDAES / idaes-pse

The IDAES Process Systems Engineering Framework
https://idaes-pse.readthedocs.io/
Other
216 stars 232 forks source link

Keras file format updates #1401

Closed rundxdi closed 1 month ago

rundxdi commented 5 months ago

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:

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.
bpaul4 commented 5 months ago

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

ksbeattie commented 5 months ago

@rundxdi any progress on this?

rundxdi commented 5 months ago

@rundxdi any progress on this?

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

rundxdi commented 4 months ago

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.
lbianchi-lbl commented 4 months ago 
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.

andrewlee94 commented 4 months ago

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.
ksbeattie commented 4 months ago

@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 commented 3 months ago

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

bpaul4 commented 2 months ago

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

rundxdi commented 2 months ago

@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-commenter commented 2 months ago

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

lbianchi-lbl commented 2 months ago

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.

bpaul4 commented 1 month ago

@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 commented 1 month ago

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

andrewlee94 commented 1 month ago

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

lbianchi-lbl commented 1 month ago

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

andrewlee94 commented 1 month ago

@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 commented 1 month ago

@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