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

[NVIDIA TF] Throw error only for non-empty function definitions. #59619

Conversation

pavanimajety
Copy link
Contributor

During Function Serialization and Deserialization with Saved Models there can be Node ops with type func with default values that have no associated name. In such cases, we shouldn't look for undefined empty strings in Function Library. This is a trivial change and helps with how Saved Models are loaded.

@google-ml-butler google-ml-butler bot added the size:XS CL Change Size: Extra Small label Feb 8, 2023
@DEKHTIARJonathan
Copy link
Contributor

@reedwm what do you think ?

@pavanimajety pavanimajety marked this pull request as ready for review February 8, 2023 22:52
@pjannaty pjannaty changed the title [BugFix] Throw error only for non-empty function definitions. [NVIDIA TF] Throw error only for non-empty function definitions. Feb 9, 2023
@gbaned gbaned requested a review from reedwm February 9, 2023 06:55
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Feb 9, 2023
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Feb 9, 2023
@reedwm reedwm requested review from k-w-w and removed request for reedwm February 10, 2023 02:28
@k-w-w
Copy link
Contributor

k-w-w commented Feb 16, 2023

Thanks for the PR! Quick question,

there can be Node ops with type func with default values that have no associated name.

When does this happen?

@DEKHTIARJonathan
Copy link
Contributor

DEKHTIARJonathan commented Feb 16, 2023

@k-w-w we tried to register an OP that can define or not an optional func.

@k-w-w
Copy link
Contributor

k-w-w commented Feb 16, 2023

I see, can you add this as a comment in the code? (# Custom ops may contain a func attr with an empty fname)

Add reason for the required change.


Add comment to both locations.
Copy link
Contributor

@k-w-w k-w-w left a comment

Choose a reason for hiding this comment

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

Thanks!

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Feb 17, 2023
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Feb 17, 2023
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Feb 17, 2023
PR Queue automation moved this from Approved by Reviewer to Closed/Rejected Feb 23, 2023
@google-ml-butler google-ml-butler bot removed awaiting review Pull request awaiting review ready to pull PR ready for merge process labels Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XS CL Change Size: Extra Small
Projects
PR Queue
  
Closed/Rejected
Development

Successfully merging this pull request may close these issues.

None yet

5 participants