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

Use INFO instead of WARNING for an unused graph input. #1235

Merged

Conversation

skottmckay
Copy link
Contributor

An unused graph input doesn't hurt anything so use INFO rather than WARNING so the message doesn't show up with the default output level of WARNING.

@skottmckay skottmckay requested a review from a team as a code owner June 17, 2019 06:21
jywu-msft
jywu-msft previously approved these changes Jun 17, 2019
@snnn
Copy link
Member

snnn commented Jun 17, 2019

It indicated the model is somehow wrong. But, now , we get this warning because of our internal graph transformers. We should fix the problem, instead of ignore the warning.

@skottmckay
Copy link
Contributor Author

It indicated the model is somehow wrong. But, now , we get this warning because of our internal graph transformers. We should fix the problem, instead of ignore the warning.

The constant folding actually relies on the cleanup of unused initializers in graph.cc so it's expected that there would be things to remove making 'warning' too sever in that case. That could be refactored though.

In other cases, whilst the model could have these unused things removed, is it 'wrong' or just sub-optimal?

@kkaranasos
Copy link
Contributor

I think it would be great to add a RemoveInitializerIfUnused method, and use it in multiple transformers (constant folding; but also in some fusions eventually as long as we don't update initializers in place, there is an ongoing discussion about that). It might also help with releasing memory gradually.
But either way, it doesn't seem to me that unused initializers should be a warning.

@snnn
Copy link
Member

snnn commented Jun 17, 2019

Without enabling the transformers, we only saw this warning in very a few models (<1%). The message indicates the converter should be improved, to not leave garbage in the model.

@askhade
Copy link
Contributor

askhade commented Jun 21, 2019

It indicated the model is somehow wrong. But, now , we get this warning because of our internal graph transformers. We should fix the problem, instead of ignore the warning.

The constant folding actually relies on the cleanup of unused initializers in graph.cc so it's expected that there would be things to remove making 'warning' too sever in that case. That could be refactored though.

In other cases, whilst the model could have these unused things removed, is it 'wrong' or just sub-optimal?

In my opinion, other cases (non transformer related) where we see these unused inputs or initializers - it is not wrong... it is sub-optimal. It would be wrong if it affects the correctness... That said we should fix converters to remove any unused initializer\input.

Right now this change looks good to me. Later we can implement the change to gradually release the memory instead of waiting till the end. This will not only eliminate such logs but it will also alleviate the peak memory increase issue.

askhade
askhade previously approved these changes Jun 21, 2019
…hat is never used, and an info level message if removing an initializer that optimization has made redundant.
@skottmckay skottmckay merged commit 07a2466 into master Jul 15, 2019
@skottmckay skottmckay deleted the skottmckay/LowerSeverityOfMessageForUnusedGraphInput branch July 15, 2019 10:29
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.

5 participants