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

Add support for named outputs in MLF archive #13704

Merged
merged 1 commit into from
Jan 9, 2023

Conversation

Mousius
Copy link
Member

@Mousius Mousius commented Jan 5, 2023

Following from #12789, this adds support for determining the output tensor name from the input model within the MLF metadata json.

Co-authored-by: Ashutosh Parkhi [email protected]

@tvm-bot
Copy link
Collaborator

tvm-bot commented Jan 5, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

  • No users to auto-tag found, no teams are specified in PR title See #10317 for details

Generated by tvm-bot

Following from apache#12789, this adds support for determining the output tensor name from the input model within the MLF metadata json.

Co-authored-by: Ashutosh Parkhi <[email protected]>
@Mousius Mousius force-pushed the mlf-named-metadata branch from fc5aec8 to b32beba Compare January 5, 2023 13:30
Copy link
Member

@mehrdadh mehrdadh left a comment

Choose a reason for hiding this comment

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

I don't think this requires an RFC, but could you please add some details of the metadata.json new structure to the PR description. Mainly explain how would someone access the output/input names for better visibility.

"test_output_d": {"size": 12, "dtype": "float32"},
}


if __name__ == "__main__":
sys.exit(pytest.main([__file__] + sys.argv[1:]))
Copy link
Member

Choose a reason for hiding this comment

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

Please update this to tvm.testing.main()

Copy link
Member Author

Choose a reason for hiding this comment

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

This feels like it'd be better done as a blanket change rather than polluting many individual patches?

Copy link
Member

Choose a reason for hiding this comment

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

either way is fine. We don't have a lint step to catch these, that's why I've trying to notify people to fix it as I see them in the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather not introduce unrelated changes into my patch, I can try a mass find and replace in a future patch? 😸

@Mousius
Copy link
Member Author

Mousius commented Jan 5, 2023

I don't think this requires an RFC, but could you please add some details of the metadata.json new structure to the PR description. Mainly explain how would someone access the output/input names for better visibility.

Hi @mehrdadh, #12789 adds the functionality in the metadata, this adds support for output names - I haven't changed the structure.

@Mousius
Copy link
Member Author

Mousius commented Jan 6, 2023

@tvm-bot rerun

@Mousius
Copy link
Member Author

Mousius commented Jan 9, 2023

@mehrdadh anything else to update here? I changed the testing hook in #13717 😸

Copy link
Contributor

@ashutosh-arm ashutosh-arm left a comment

Choose a reason for hiding this comment

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

Thanks @Mousius! Looks good to me. So, if we merge this PR, can we close #13655 which adds a similar test to test only the sizes?

@Mousius
Copy link
Member Author

Mousius commented Jan 9, 2023

@ashutosh-arm I think we still need #13655 for the checking of the actual constant which was broken before? this just checks the JSON

Copy link
Member

@mehrdadh mehrdadh left a comment

Choose a reason for hiding this comment

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

LGTM! thanks!

@mehrdadh mehrdadh merged commit 329584b into apache:main Jan 9, 2023
driazati added a commit to driazati/tvm that referenced this pull request Jan 9, 2023
@driazati
Copy link
Member

driazati commented Jan 9, 2023

fyi @Mousius @mehrdadh this seems to have merge conflicted with something once it was merged in 329584b so CI is now failing, I have #13739 up to revert it, we can wait and see if that fixes the failures we're seeing on main and merge it if so, then rebase this.

example failure: https://ci.tlcpack.ai/blue/organizations/jenkins/tvm-cortexm/detail/main/115/pipeline

mehrdadh added a commit that referenced this pull request Jan 10, 2023
This PR fixes error introduced by #13704
fzi-peccia pushed a commit to fzi-peccia/tvm that referenced this pull request Mar 27, 2023
Following from apache#12789, this adds support for determining the output tensor name from the input model within the MLF metadata json.

Co-authored-by: Ashutosh Parkhi <[email protected]>
fzi-peccia pushed a commit to fzi-peccia/tvm that referenced this pull request Mar 27, 2023
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