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

Fix AOT bundle CMake build and regression tests #3027

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@nickgg
Copy link
Contributor

commented May 31, 2019

Summary: The AOT bundle examples had multiple makefiles, of which the CMake version did not work properly. This fixes the CMakeLists for the two bundle examples (plus the two extra quantized versions) and deletes the extra Makefile.

We did regression test the mnist bundle using the non-CMake makefile, but not resnet50 - I've modified the CI script to test both bundles using the CMakeList version.

Documentation: updated AOT.md to remove references to the Run*Bundle targets which never worked as described and I have removed.

fixes #3013

Test Plan: ran manually, waiting to see if it works in CI.

@nickgg nickgg force-pushed the nickgg:aot-wip2 branch 2 times, most recently from 00fad07 to 89eb25e May 31, 2019

@nickgg

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

fixed portability of uint64_t printfs in bundle example code

@nickgg nickgg force-pushed the nickgg:aot-wip2 branch from 89eb25e to bd29a72 May 31, 2019

@jfix71

jfix71 approved these changes May 31, 2019

Copy link
Contributor

left a comment

LGTM, modulo CI working correctly. I'm surprised we were already downloading all of the C2 models. I'm not sure why it's necessary -- perhaps we can just download those we are testing?

@nickgg nickgg force-pushed the nickgg:aot-wip2 branch from bd29a72 to 9d91eba May 31, 2019

@nickgg

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

We download what we're using for the bundles, not 100% sure why we need all models. My guess was because we ran run.sh but looks like we don't so maybe we can remove the model download?

@facebook-github-bot
Copy link

left a comment

@nickgg has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot

This comment has been minimized.

Copy link

commented May 31, 2019

@nickgg merged this pull request in 1af031c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.