-
Notifications
You must be signed in to change notification settings - Fork 234
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
Adds ONNX Surrogate support from OMLT #1308
base: main
Are you sure you want to change the base?
Conversation
Likely want to refactor this later
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1308 +/- ##
=======================================
Coverage 77.43% 77.43%
=======================================
Files 390 392 +2
Lines 63758 63844 +86
Branches 11737 11749 +12
=======================================
+ Hits 49373 49440 +67
- Misses 11841 11852 +11
- Partials 2544 2552 +8 ☔ View full report in Codecov by Sentry. |
@avdudchenko can you look into the pylint and rtd test failures? |
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.
@avdudchenko Generally this all looks good. I have a couple very minor questions and test failures need to be resolved, then I can approve it.
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.
Changes to this file all look 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.
Changes to this file all look good
|
||
# TODO: remove this once new OMLT 1.2 is made available and includes tanh support | ||
# overrides default available activation functions for ONNX, tanh is not listed in 1.1 but is supported | ||
omltio.onnx_parser._ACTIVATION_OP_TYPES = [ |
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.
Does this need catches for anything?
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.
Not sure best catches to implement here, thoughts on just checking for OMLT version? (It should not be needed with future OMLT releases.
) | ||
self.populate_block_with_net(block, formulation_object) | ||
|
||
def evaluate_surrogate(self, inputs): |
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.
Why is this present but only raises NotImplementError?
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.
Left it here for future implementation, but setting up ONNX model to solve outside idaes did not seem trivial at time of first commit. (e.g. there did not seem to be something as simple as _keras_model.predict.
@avdudchenko @rundxdi should this be part of the next (now May) release? |
@avdudchenko @rundxdi now would be a good time to get this on the Aug release, if you can find a reviewer for it |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1308 +/- ##
==========================================
- Coverage 76.98% 76.98% -0.01%
==========================================
Files 382 384 +2
Lines 61913 61995 +82
Branches 10130 10136 +6
==========================================
+ Hits 47665 47728 +63
- Misses 11848 11859 +11
- Partials 2400 2408 +8 ☔ View full report in Codecov by Sentry. |
@avdudchenko, this is now on the Nov 2024 release board. Can progress be picked up on this now? |
…-pse into onnx_surrogate
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.
Finalizing my review -- looks good at this point
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 the changes implemented here are good. Not critical to fix, there are a few places in the ONNX file that refer to "keras" instead of "onnx".
@avdudchenko Can you look at the test failures and see if they can be fixed? |
Fixes
This adds ONNX surrogate model object and slightly refactors the current Keras surrogate model to support multiple omlt model types.
Summary/Motivation:
Currently, if one generates a NN model using PyTorch, or any other method for use in IDAES and depended project (e.g. WaterTAP) we must export the model to ONNX format, and then to keras format to use in IDAES, which makes little sense.
This PR adds native support to onnx model format provided through OMLT.
Changes proposed in this PR:
-move out part of Keras functions into omlt_surrogate_base.py to enable simple support for multiple omlt import types
-adds ONNXSurrogate object that can load onnx model directly in IDAES flowsheet block
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: