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

[testing][py_converter] Enhance py_converter to better support entire modules #13769

Merged
merged 8 commits into from
Feb 15, 2023

Conversation

slyubomirsky
Copy link
Contributor

@slyubomirsky slyubomirsky commented Jan 12, 2023

This PR makes a few improvements to py_converter to make it more useful for fuzz testing, especially when running larger modules.

In particular, these changes are to support returning the definition of a global var directly (e.g., if you do run_as_python(main_var, mod=mod), the result will be a function corresponding to mod["main"]) and to correct two bugs in the previous implementation:

  • Previously, it was not possible to insert a function into a runtime container object like an ADT. This was because the converter was simply compiling Relay functions into Python functions. This change solves this problem by registering the functions into PackedFuncs. However, another fix was also needed: Even though PackedFunc is an ObjectRef in C++, the Python bindings do not recognize PackedFuncs as Objects, so the code now calls the FFI API tuple constructor directly.
  • The implementation relied on IRModule.from_expr to wrap passed in expressions in a module. However, from_expr will not overwrite the main function if one is passed in via the functions argument. Thus, if the user passed in a module that already had a main function defined, the wrapping would be done incorrectly and result in the main being copied many times. This PR corrects this error by not assuming that the name main will be available and instead constructing a new module with a reserved name for the target.

None of the cases above had been tested before (there are now tests included)

@tvm-bot
Copy link
Collaborator

tvm-bot commented Jan 12, 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 tag found in teams: testing, py_converter See #10317 for details

Generated by tvm-bot

@slyubomirsky
Copy link
Contributor Author

Please review @junrushao (or tag other reviewers)

@slyubomirsky
Copy link
Contributor Author

Hm, this test worked in my fork. Wonder why it fails in mainline

@slyubomirsky
Copy link
Contributor Author

slyubomirsky commented Jan 12, 2023

The problem seems to be that this cast on mainline will not convince Python that the result is an opaque Object rather than a PackedFunc. Not sure how to change it:

TVM_REGISTER_GLOBAL("runtime.CastPackedFuncToObject").set_body_typed([](const PackedFunc& pf) {
  return Downcast<ObjectRef>(pf);
});

Any advice?

@slyubomirsky
Copy link
Contributor Author

@tqchen helpfully advised that the issue stems from passing a list containing a PackedFunc to the constructor. Calling the ADT constructor via the FFI API directly (while a little strange) gets around this issue. That seems like a good fix that does not require disrupting other sections of the codebase, so I will implement it

@slyubomirsky
Copy link
Contributor Author

The CI error appears to be spurious.

@slyubomirsky
Copy link
Contributor Author

@tvm-bot rerun

@slyubomirsky
Copy link
Contributor Author

I think there's been another spurious failure.

@slyubomirsky
Copy link
Contributor Author

@tvm-bot rerun

@slyubomirsky slyubomirsky force-pushed the py_converter-updates branch 2 times, most recently from c7ee534 to edd3ee3 Compare January 23, 2023 21:41
@junrushao
Copy link
Member

CC @Hzfengsy would you like to review this PR? Thanks!

Copy link
Member

@Hzfengsy Hzfengsy 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 a lot, @slyubomirsky!

@Hzfengsy Hzfengsy merged commit e516eaa into apache:main Feb 15, 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.

4 participants