Skip to content
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

Detect multiple Lambda layers #61

Merged
merged 4 commits into from
Dec 1, 2023
Merged

Conversation

mehrinkiani
Copy link
Member

At the moment, ModelScan can detect a Lambda layer in a Keras model. Though, ModelScan does not report if more than one Lambda layers are found in a given Keras model, i.e. one or more Lambda layers get reported as a single issue.

This PR adds functionality in ModelScan to report multiple Lambda layers if they exist in a Keras model.

@mehrinkiani mehrinkiani requested a review from a team November 22, 2023 15:11
@mehrinkiani mehrinkiani self-assigned this Nov 29, 2023
]
if layer["class_name"] == "Lambda"
]
model_config = json.loads(model_hdf5.attrs["model_config"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a good idea here to handle JSONDecoderError in case parsing JSON fails for model_config.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thank you, I have added try-except block to handle potential errors during JSON loading.

layers = model_config["config"]["layers"]
lambda_layers = []
for layer in layers:
if layer["class_name"] == "Lambda":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also user layer.get(..) here as it is being used in the next line?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review, I have updated it to layer.get(..)

Copy link
Member

@iamfaisalkhan iamfaisalkhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mehrinkiani mehrinkiani merged commit 07958d0 into main Dec 1, 2023
8 checks passed
@mehrinkiani mehrinkiani deleted the detect-multiple-lambda-layers branch December 1, 2023 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants