Skip to content

Conversation

@the-david-oy
Copy link
Contributor

@the-david-oy the-david-oy commented Feb 14, 2023

Previously, if a signature def tag was not found, a random tag would be selected. This is not desirable behavior as it masks errors and users have no way to know a different tag was used without looking at the logs.

This PR removes that behavior in our TensorFlow mirror.

It does not make sense to add a test for the absence of a feature. To ensure these changes are successful, I tested locally and received the below results.

Before updates, model with wrong tag in config loads:
image

After updates, model with wrong tag in config fails to load:
image

@the-david-oy the-david-oy changed the title Remove signature tag random selection when not found Remove random selection fallback when signature def not found Feb 14, 2023
@tanmayv25
Copy link
Contributor

@dyastremsky This seems like a breaking change. Before this change, users didn't have to care for what signature def are present in their model and Triton will serve model with any signature that it finds. But I agree that we should throw an error when user specified signature_def is not present in the model instead of loading random def.

Can we throw an error of signature_def not found iff user has specified a signature_def explicitly? My concern is that some model may have "serving_default" (default signature def) named as something else and users might start running into issues with this change.

@the-david-oy
Copy link
Contributor Author

@tanmayv25 Sure, good point. I've modified the code to still randomly select a tag if one was not specified, similar to previous behavior.

Tested the same cases as above with similar results, plus the case where no tag is specified:
image

@the-david-oy
Copy link
Contributor Author

the-david-oy commented Feb 21, 2023

@tanmayv25, please review when you get a chance. I'll merge this once the associated version of TensorFlow is released (23.03, maybe 23.02).

@the-david-oy the-david-oy merged commit d12bf3f into main Apr 3, 2023
@the-david-oy the-david-oy deleted the dyas-tag-random branch April 3, 2023 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants