-
Notifications
You must be signed in to change notification settings - Fork 228
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
Keras file format updates #1401
Conversation
@@ -263,7 +263,7 @@ def save_to_folder(self, keras_folder_name): | |||
The name of the folder to contain the Keras model and additional | |||
IDAES metadata | |||
""" | |||
self._keras_model.save(keras_folder_name) | |||
self._keras_model.save(os.path.join(keras_folder_name, "idaes_keras_model.keras")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the file name be hard-coded here, or should we make this an input from the user (either optional or required).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support letting users pass a string for the file name, so something like
self._keras_model.save(os.path.join(keras_folder_name, "idaes_keras_model.keras")) | |
self._keras_model.save(os.path.join(keras_folder_name, keras_file_name + ".keras")) |
where keras_file_name
is an input to the save method. I don't know if we still need to specify the folder name, or if a single string for the entire path is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The folder name is used to save/load some model weights and some idaes specific information separate from the filename. But I think the file name should be rename-able as you say -- making that change now
@rundxdi any progress on this? |
Nothing this week -- still trying to figure out plotting tests. |
I have two remaining issues:
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
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:
|
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @rundxdi for investigating and implementing the fix. The changes to the Keras/OMLT calls and file writing look good.
I have a couple questions. First, the models previously import from a folder containing saved_model.pb
, keras_metadata.pb
, and idaes_info.json
files and a variables/
folder containing variables.index
and variables.data-00000-of-00001
files. Are these still necessary, or can we remove them in favor of the .keras
files? I've seen in my research into Keras 3 that it now doesn't support SavedModel files and includes a read-only TFSM format instead.
Second, do we want to look into the Python 3.8 failures? Since it isn't occurring in the newer Python environments, it may just be a compatibility issue that's best skipping in that one environment.
@rundxdi when you have a moment, could you please provide a status update? What is required to finalize this? |
Codecov ReportAttention: Patch coverage is
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 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good now, thanks @rundxdi!
Before merging this, we should remove Python 3.8 from the CI and see if the tests pass without needing to mark them as |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bpaul4 I was waiting for the Python 3.8 issue to be merged to make sure the tests passed. I have just one minor request.
@@ -53,6 +53,7 @@ def build_rom(): | |||
|
|||
|
|||
@pytest.mark.unit | |||
@pytest.mark.xfail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add an inline comment to explain why this is xfailed
. I know this is mentioned in the deprecation warning in the code, but having comments here will help reinforce that this issue needs to be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add the comments now. @AlexNoring is aware of the SOFC surrogate issue and will continue the discussion here: #1431
@andrewlee94 how would you suggest handling the failing example integration tests? The failures are resolved as part of IDAES/examples#128. |
@bpaul4 I would say we need to get that examples PR merged first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly minor comments, questions, and suggestions.
@@ -53,6 +53,7 @@ def build_rom(): | |||
|
|||
|
|||
@pytest.mark.unit | |||
@pytest.mark.xfail # test xfailed due to out-of-date, incompatible Keras surrogates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: use the reason
kwarg instead of a comment allows the message to be displayed when running the tests, increasing its visibility:
@pytest.mark.xfail # test xfailed due to out-of-date, incompatible Keras surrogates | |
@pytest.mark.xfail(reason="test xfailed due to out-of-date, incompatible Keras surrogates") |
@@ -78,6 +79,7 @@ def test_basic_build(build_rom): | |||
|
|||
@pytest.mark.skipif(solver is None, reason="Solver not available") | |||
@pytest.mark.component | |||
@pytest.mark.xfail # test xfailed due to out-of-date, incompatible Keras surrogates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: use the reason
kwarg instead of a comment allows the message to be displayed when running the tests, increasing its visibility:
@pytest.mark.xfail # test xfailed due to out-of-date, incompatible Keras surrogates | |
@pytest.mark.xfail(reason="test xfailed due to out-of-date, incompatible Keras surrogates") |
@@ -97,9 +99,20 @@ def test_initialize(build_rom): | |||
|
|||
@pytest.mark.skipif(solver is None, reason="Solver not available") | |||
@pytest.mark.component | |||
@pytest.mark.xfail # test xfailed due to out-of-date, incompatible Keras surrogates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: use the reason
kwarg instead of a comment allows the message to be displayed when running the tests, increasing its visibility:
@pytest.mark.xfail # test xfailed due to out-of-date, incompatible Keras surrogates | |
@pytest.mark.xfail(reason="test xfailed due to out-of-date, incompatible Keras surrogates") |
msg = "Tests for sofc_keras_surrogate.py have started failing. The code will be removed no early than August if it is not fixed." | ||
deprecation_warning(msg=msg, logger=_log, version="2.5.0", remove_in="3.0.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
msg = "Tests for sofc_keras_surrogate.py have started failing. The code will be removed no early than August if it is not fixed." | |
deprecation_warning(msg=msg, logger=_log, version="2.5.0", remove_in="3.0.0") | |
msg = "Tests for sofc_keras_surrogate.py have started failing following Tensorflow/Keras version updates (see IDAES/idaes-pse#1401). The code will be removed no earlier than August 2024 if it is not fixed." | |
deprecation_warning(msg=msg, logger=_log, version="2.5.0", remove_in="3.0.0") |
@@ -93,7 +93,7 @@ class ExtraDependencies: | |||
] | |||
omlt = [ | |||
"omlt==1.1", # fix the version for now as package evolves | |||
'tensorflow < 2.16.1 ; python_version < "3.12"', | |||
"tensorflow", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: given the changes in this PR, would it make sense to require a minimum version of 2.16.1 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that would be appropriate. although removing the tag will automatically search for the latest version. Pinning a minimum version to support Keras 3 would be a good safety net.
self._keras_model.save( | ||
os.path.join(keras_folder_name, keras_model_name + ".keras") | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: since Keras seems to support pathlib.Path
objects now, this could be rewritten to be slightly more concise/readable (requires replacing import os.path
with from pathlib import Path
at the top of the file):
self._keras_model.save( | |
os.path.join(keras_folder_name, keras_model_name + ".keras") | |
) | |
self._keras_model.save( | |
Path(keras_folder_name, keras_model_name).with_suffix(".keras") | |
) |
|
||
keras_model = keras.models.load_model( | ||
os.path.join(keras_folder_name, keras_model_name + ".keras") | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comments for using pathlib.Path
instead of os.path
.
nn.save_weights(os.path.join(path, "{}.h5".format(name))) | ||
nn.save(os.path.join(path, "{}.keras".format(name))) | ||
nn.save_weights(os.path.join(path, "{}.weights.h5".format(name))) | ||
|
||
|
||
def load_keras_json_hd5(path, name): | ||
with open(os.path.join(path, "{}.json".format(name)), "r") as json_file: | ||
json_model = json_file.read() | ||
nn = keras.models.model_from_json(json_model) | ||
nn.load_weights(os.path.join(path, "{}.h5".format(name))) | ||
nn = keras.models.load_model(os.path.join(path, "{}.keras".format(name))) | ||
nn.load_weights(os.path.join(path, "{}.weights.h5".format(name))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments above for using pathlib.Path
instead of os.path
.
* update save, load syntax * regenerate outdated notebooks * generate new model files * remove old keras files * Test using IDAES/idaes-pse#1401 * Update version constraint for TensorFlow * Address Python 3.8 failures due to Tensorflow 2.16.1 not being available * Remove exclusion for Python 3.12 for Tensorflow * Remove Python 3.8 support * Restore installing idaes-pse from main branch --------- Co-authored-by: Ludovico Bianchi <lbianchi@lbl.gov>
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. |
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: |
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: