Skip to content

Conversation

@apivovarov
Copy link
Contributor

@apivovarov apivovarov commented Mar 27, 2019

I tried to run autotvm for custom ARM device recently and found that current autotvm requires edge device to have build tools (gcc, g++, ld) in order to link tar(.o) file to .so file.

It might be a situation when custom ARM devices has minimal linux installation which does not have gcc, g++ and ld tools installed. Also it might be missing package managers like apt.

I found that it is possible to cross compile and export shared object (.so file) for edge device on tracker side using cc parameter in lib.export_library.

This PR replaces default_build_func and android_ndk_build_func identical methods with function make_build_func returning build_func.
I also updated tune_relay_mobile_gpu.py tutorial to demonstrate the use of make_build_func to generate so file instead of default tar file

@yzhliu
Copy link
Member

yzhliu commented Mar 27, 2019

I feel it might be more proper to have a new builder, e.g., CrossBuilder. @merrymercy please comment.

@tqchen
Copy link
Member

tqchen commented Mar 27, 2019

You don’t have to pass these argument in. A growing number of args is exactly what we want to prevent from happening here. Instead, you can direct pass in a callable which is a functor that has those options baked in

@kevinthesun
Copy link
Contributor

Maybe we can just create more built-in build_func and register them with proper name.

@tqchen
Copy link
Member

tqchen commented Mar 27, 2019

We can add some of them if they are common. if there is still a lot of things to be configured, my guess is passing in a custom callback will be much cleaner

@apivovarov
Copy link
Contributor Author

@tqchen , @kevinthesun, @yzhliu Thank you for your quick feedbacks!
When I look at the existing code second time I found that default_build_func and android_ndk_build_func methods are 99% identical.
I replaced them with DefaultBuilder class which has build_func method and three class parameters:

  • file_ext="tar"
  • fcompile=None
  • **export_library_kwargs

That solutions allows me to replace default_build_func and android_ndk_build_func methods with single DefaultBuilder.build_func method.
Also I added export_library_kwargs class parameter which allows users to specify export_library options. I put the example of how to pass cc option to DefaultBuilder - See tune_relay_mobile_gpu.py tutorial.
Just uploaded the second revision of the PR

@tqchen
Copy link
Member

tqchen commented Mar 27, 2019

Let us stick with the original API and not introducing the new arguments.

So with the existing API, you can already define a function my_cross_build with the extra arguments in and pass that in as build_func directly. The callback function is really powerful and it allows you to define arbitrary ways of compilation.

The reason why we do not want to add new options to the Builder, is that these options only carry a few cases and sometimes users want more. Because build_func can already be functors, they can be customized to contain arbitrary code.

For example, we can just define my_cross_build in users' code and just pass that in as an argument.

def my_cross_build(measure_input, tmp_dir, **kwargs):
    tic = time.time()
    try:
        filename = os.path.join(tmp_dir, "tmp_func_%0x.so" % getrandbits(64))
        func, arg_info = _build_func_common(measure_input, **kwargs)
        func.export_library(filename, cc, **extra_options)
    except Exception as e:  
        return BuildResult(None, None, e, time.time() - tic)
    return BuildResult(filename, arg_info, None, time.time() - tic)

If we want to complain that this is still too complicated, then we should do something to make it easier to do clean callback functional specification. Or perhaps create a functor that builds the functions

@apivovarov
Copy link
Contributor Author

apivovarov commented Mar 28, 2019

@tqchen Can you look at the second (2nd) revision of the PR I posted on Mar 27?
It does not change ANY API.

I just replaced default_build_func and android_ndk_build_func methods with class DefaultBuilder which has single build_func method.

@FrozenGene
Copy link
Member

My way is to use_ndk (i.e. build_func='ndk') and set TVM_NDK_CC. Then we use lib.export_library(path, ndk.create_shared). If we have one better wrapper, it also be nice.

@apivovarov
Copy link
Contributor Author

@tqchen I just submitted 3rd revision of the PR

  • No modifications in any API
  • No modifications in __init__.py files

This PR just replaces two identical methods default_build_func and android_ndk_build_func with class DefaultBuilder having single method build_func.

except Exception as e: # pylint: disable=broad-except
return BuildResult(None, None, e, time.time() - tic)
return BuildResult(filename, arg_info, None, time.time() - tic)
class DefaultBuilder:
Copy link
Member

@merrymercy merrymercy Mar 28, 2019

Choose a reason for hiding this comment

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

Since we already have LocalBuilder and RPCBuilder, which work on a different level, this name is very confusing.
We can remove this class and implement it as a function create_build_func

def create_build_func(file_ext, fcompile, ...):
    def build_func_(...):
        ...
    return build_func

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree on the name issue. I renamed DefaultBuilder to BuildFuncFactory. I think that using Class should be fine here. It incapsulates class params and build_func method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced class BuildFuncFactory with function make_build_func

return BuildResult(filename, arg_info, None, time.time() - tic)
class BuildFuncFactory:
def __init__(self, file_ext="tar", fcompile=None, **export_library_kwargs):
"""
Copy link
Member

@merrymercy merrymercy Mar 28, 2019

Choose a reason for hiding this comment

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

put this doc string below the line of class BuildFuncFactory: to fix the lint error.
Typically, we put the doc string of __init__ function below class rather than the __init__ function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the docs

@tqchen
Copy link
Member

tqchen commented Mar 29, 2019

OK. You don't really need to have BuildFactory as a class. In python you can return closures, which means you can do things functionally.

def make_build_func(args):
    def my_build_func():
        #   code
    return my_build_func

This being said, we actually can get a better solution to this problem.

What is a better solution

The real reason why we are doing such a level of indirection was really due to the fact that the tvm.cc.create_shared and export_library do not play well with each other . The reason that we need another level of indirection here is that we need to know the extension file type and the build_args which is separated by _build_common in the code. We can definitely get it cleaner by somehow suggests the extension type in the build function.

Consider the following code, which is much cleaner

import tvm
from tvm.contrib import cc

def my_build_func(output, objects):
     cc.create_shared(output, objects, cc="myg++", **mykwargs)
# annotate the library output format
my_build_func.lib_format = "so"
builder=autotvm.LocalBuilder(my_build_func)

And we enhance the export_library to support the following case

# notice no suffix here
#  the default lib_format suffix will be attached if hasattr(my_build_func, "lib_format")
# otherwise a ValueError is raised.
lib.export_library("mylib", my_build_func)

Now we can even get rid of the build_common, because we can just take cc.create_shared as the build_func (assuming we also set the right lib_format).

What is even better, we can enhance create_shared to return a partial functor if you do not specify output and objects(this might go a bit far but may worth the effort)

builder=autotvm.LocalBuilder(cc.create_shared(cc="myg++", options=myopts))

Since this will require some changes to AutoTVM API, cc @merrymercy for thoughts

@apivovarov
Copy link
Contributor Author

@tqchen @merrymercy I replaced class BuildFuncFactory with function make_build_func.
Can you accept this PR as a working solution which allows users to easily switch from tar to so file for autotvm.
I will open new PR for "better solution" code refactoring proposed by Tianqi.

@tqchen
Copy link
Member

tqchen commented Mar 29, 2019

If possible, it would be great if we can directly go with the better solution as per https://docs.tvm.ai/contribute/code_review.html#hold-the-highest-standard

Mainly because the better solution is not too hard to get and we don't have to change the solution twice. perhaps @merrymercy can also coordinate/help on this.

@tqchen
Copy link
Member

tqchen commented Mar 29, 2019

BTW, I find that there are useful design lessons that can be highlighted through the conversations of this PR. Mainly on

  • Callback vs adding more arguments
  • Factory class vs functional style
  • Reduce abstractions vs adding layers of abstraction

It would be really great we can learn from this, and summarize the lessons, and contribute that into design docs.

@apivovarov
Copy link
Contributor Author

@merrymercy After looking at existing measure_methods.py and cc.py code one more time I think that the refactoring proposed by Tianqi in "better solution" requires autotvm retesting on gpu (OpenCL and CUDA) because _build_func_common which Tianqi suggested to remove deals with check_gpu and cuda_arch parameters.
Another issue is that cc.create_shared automatically determines platform using sys but in case of autotvm the compilation should be done not for tracker platform but for edge device platform.
I think that the change requested by Tianqi in "better solution" should be done in a separate PR and requires dedicated time to implement it and test.
Given this situation can I ask you to merge this PR in its current state.

@tqchen
Copy link
Member

tqchen commented Mar 29, 2019

It would be great to take a few days to get the better solution out:) while it might take a bit more time, but it is always good to go with the best design we have. New code changes always comes with technical debt and it is important to avoid it.

The reason that we are doing code review and discussion(instead of directly send in another PR) is to find time to improve the design and make everyone learn.

Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

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

Additional comments on the tutorials, besides the design improvement mentioned in the set of discussions :)


# Cross compiler which will be used to export library to shared object (.so file) for edge device
# Edge device does not need to have build-essential package (gcc, g++, ld, etc) to be installed
cc = 'aarch64-linux-gnu-g++'
Copy link
Member

Choose a reason for hiding this comment

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

We need to add more follow up instructions on how to install such cross compiler as a part of note block, set the following flags to use cross gcc and default to False(because the on device compiler is still the easiest in many cases)

Copy link
Contributor Author

@apivovarov apivovarov Mar 29, 2019

Choose a reason for hiding this comment

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

I updated the PR:

  • set use_cross_compiler_to_export_lib=False by default
  • added instructions on how to install armv7/8 cross-compilers

Copy link
Member

Choose a reason for hiding this comment

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

@apivovarov
Copy link
Contributor Author

@merrymercy Looks like I addressed all of the comment above except of code refactoring for measure_methods.py. My intention was to share what I did to run autotune for edge device without g++/gcc/ld installed. I think measure_methods.py code refactoring should be done in a separate activity/PR.


# Cross compiler which will be used to export library to shared object (.so file) for edge device
# Edge device does not need to have build-essential package (gcc, g++, ld, etc) to be installed
cc = 'aarch64-linux-gnu-g++'
Copy link
Member

Choose a reason for hiding this comment

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

@tqchen
Copy link
Member

tqchen commented Mar 30, 2019

For reference, I have created #2927 which implemented the proposed better solution.

@tqchen
Copy link
Member

tqchen commented Mar 30, 2019

Let me explain a bit why we don't want to merge the proposed change before the refactor.

New features(use cross-compiler) is always exciting but also brings technical debt(backward compatibility and how the developer is going to use it). Given that the refactored ver is better and will also change the way things are being used. It is better to not introduce the new feature and always aim for the best solution.

@tqchen tqchen added status: need update need update based on feedbacks and removed status: need review labels Mar 30, 2019
@tqchen
Copy link
Member

tqchen commented Mar 30, 2019

@apivovarov please rebase against the master and update the tutorial comments with note and code blocks

@tqchen tqchen changed the title Add ability to specify export_library kwargs for autotvm Add Example to Support Cross Compiler Mar 30, 2019
@tqchen tqchen changed the title Add Example to Support Cross Compiler Add Example for Cross Compilers Mar 30, 2019
@apivovarov apivovarov closed this Apr 1, 2019
@apivovarov apivovarov deleted the autotvm_lib_exp branch April 1, 2019 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: need update need update based on feedbacks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants