-
Notifications
You must be signed in to change notification settings - Fork 54
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
Machine Learning & Artificial Intelligence Plugin #995
Conversation
…l_ai_plugin_test
…l_ai_plugin_test
Add GUI tests for ML/AI plugin
@lbianchi-lbl your tests and CI updates are very well written, and it appears to have resolved all remaining GitHub Actions issues - thank you for your continuous efforts on this task. @anujad95 @jmorgan29 @MAZamarripa please provide any additional feedback you have on the Plugin code or the new tests so we may merge these updates soon; @anujad95 and I previously tested the tool on our local machines with no issues. |
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.
Following the discussion today during the dev call, we came to the conclusion that making the exception handling more specific could be a low-effort way to make unexpected errors more visible. I've left review comments where I think it would make sense to do this, as well as a few suggestions on the implementation.
foqus_lib/framework/graph/node.py
Outdated
import tensorflow as tf | ||
|
||
load = tf.keras.models.load_model | ||
except: |
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.
except: | |
except ImportError: | |
# if TensorFlow is not available, create a proxy function that will raise an exception | |
# whenever code tries to use `load()` at runtime | |
def load(*args, **kwargs): | |
raise RuntimeError(f"`load()` was called with args={args}, kwargs={kwargs} but `tensorflow` is not available") |
foqus_lib/framework/graph/node.py
Outdated
for i in range(np.shape(self.model.inputs[0])[1]): | ||
try: | ||
input_label = self.model.layers[1].input_labels[i] | ||
except: |
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.
Suggestions to make exceptions more specific:
- Use
except MyExceptionType:
instead of "bareexcept
- Add a comment with more details on when that exception might occur (e.g. "this happens if the input labels in the model do not match those stored in the file" (just an example))
foqus_lib/framework/graph/node.py
Outdated
input_min = 0 # not necessarily a good default | ||
try: | ||
input_max = self.model.layers[1].input_bounds[input_label][1] | ||
except: |
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.
TODO: make except
clause more specific (see above for suggestions)
foqus_lib/framework/graph/node.py
Outdated
for j in range(np.shape(self.model.outputs[0])[1]): | ||
try: | ||
output_label = self.model.layers[1].output_labels[j] | ||
except: |
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.
TODO: make except
clause more specific (see above for suggestions)
foqus_lib/framework/graph/node.py
Outdated
output_label = "z" + str(j + 1) | ||
try: | ||
output_min = self.model.layers[1].output_bounds[output_label][0] | ||
except: |
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.
TODO: make except
clause more specific (see above for suggestions)
foqus_lib/framework/graph/node.py
Outdated
output_min = 0 # not necessarily a good default | ||
try: | ||
output_max = self.model.layers[1].output_bounds[output_label][1] | ||
except: |
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.
TODO: make except
clause more specific (see above for suggestions)
foqus_lib/framework/graph/node.py
Outdated
# check if user passed a model for normalized data - FOQUS will automatically scale/un-scale | ||
try: # if attribute exists, user has specified a model form | ||
self.normalized = self.model.layers[1].normalized | ||
except: # otherwise user did not pass a normalized model |
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.
TODO: make except
clause more specific (see above for suggestions)
foqus_lib/framework/graph/node.py
Outdated
str(self.modelName): getattr(module, str(self.modelName)) | ||
}, | ||
) | ||
except: # try to load model without custom layer |
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.
TODO: make except
clause more specific (see above for suggestions)
foqus_lib/framework/graph/node.py
Outdated
) | ||
except: # try to load model without custom layer | ||
self.model = load(str(self.modelName) + ".h5") | ||
os.chdir(cwd) # reset to original working directory |
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.
Resetting the working dir inside a finally
block ensures that it is always restored, regardless of what exceptions are thrown in the previous lines.
os.chdir(cwd) # reset to original working directory | |
finally: | |
os.chdir(cwd) # reset to original working directory |
foqus_lib/framework/graph/node.py
Outdated
except: # try to load model without custom layer | ||
self.model = load(str(self.modelName) + ".h5") | ||
os.chdir(cwd) # reset to original working directory |
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 above for exception and os.chdir()
handling
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.
Thanks @bpaul4 for adding the more specific exceptions and especially the detailed messages, I think they're really helpful.
I've left two very minor corrections, and they're completely "take-it-or-leave-it", so feel free to dismiss them and I'm happy to approve either way.
foqus_lib/framework/graph/node.py
Outdated
# if TensorFlow is not available, create a proxy function that will raise | ||
# an exception whenever code tries to use `load()` at runtime | ||
def load(*args, **kwargs): | ||
raise RuntimeError(f"`load()` was called with args={args}," |
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.
Minor suggestion, but in retrospective ModuleNotFoundError
would be a better fit than RuntimeError
here.
foqus_lib/framework/graph/node.py
Outdated
@@ -106,15 +109,33 @@ def __init__(self, model): | |||
for i in range(np.shape(self.model.inputs[0])[1]): | |||
try: | |||
input_label = self.model.layers[1].input_labels[i] | |||
except: | |||
except AttributeError: | |||
logging.getLogger("foqus." + __name__).info( |
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 these extended messages are great, they add lots of useful context both for users and other developers.
One minor suggestion: instead of repeating the logging.getLogger("foqus." + __name__)
each time, you could create a logger object at the module level, e.g. _logger = logging.getLogger("foqus." + __name__)
after the import
statements on Line 40, and then use that object throughout the rest of the module, i.e. _logger.info(...)
Thanks @lbianchi-lbl, I agree with your final two suggestions. I'll make those changes and push again for final approval. |
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.
Looks good!
I believe that the last round of reviews and changes covers what we discussed on Tuesday, so I'm going ahead and merge this in. Thanks to all those involved and @bpaul4 in particular for bringing this over the finish line! |
Building off FOQUS's existing plugin to load custom Pymodel scripts, this tool allows users to import Tensorflow Keras models into FOQUS nodes. As a first draft, the tool enables new flowsheet capabilities using the same UI as the Pymodel plugin: