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.

Sign up for GitHub

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

Jump to bottom

Add standalone_crt/ to be part of the wheel package, when available. #9005

Merged
merged 1 commit into from
Sep 15, 2021
Merged

Add standalone_crt/ to be part of the wheel package, when available. #9005

merged 1 commit into from
Sep 15, 2021

Conversation

leandron
Copy link
Contributor

Add standalone_crt/ to be part of the wheel package, when available.

  • When using a packaged TVM such as tlcpack, it is impossible to run tvm.micro.get_standalone_crt_dir(), because the subtree standalone_crt/ is not available.

  • This patch adds standalone_crt/ as data_files, so that they can be picked up by _ffi.libinfo.find_lib_path() and therefore be found when tvm.micro.get_standalone_crt_dir() is invoked.

There is a counterpart change on tlc-pack/tlcpack#73 to enable this in the official packaging.

cc @areusch @manupa-arm @Mousius @tqchen

@leandron
Add standalone_crt/ to be part of the wheel package, when available.
2fa08d2 
* When using a packaged TVM such as tlcpack, it is impossible to run
  `tvm.micro.get_standalone_crt_dir()`, because the subtree
  `standalone_crt/` is not available.

* This patch adds `standalone_crt/` as `data_files`, so that they
  can be picked up by _ffi.libinfo.find_lib_path() and therefore
  be found when `tvm.micro.get_standalone_crt_dir()` is invoked.
@leandron leandron mentioned this pull request Sep 14, 2021
Merged
@leandron
Copy link
Contributor Author

also cc @tkonolige @comaniac in case you can have a look, as I'm keen to have this issue fixed

tkonolige
tkonolige approved these changes Sep 14, 2021
View reviewed changes
Copy link
Contributor

@tkonolige tkonolige left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this looks good, but I've only worked a little bit with setup.py.

One recommendation might be to rename LIB_PATH and get_lib_path to DATA_FILES and get_data_files() respectively because they no longer refer only to libraries.

manupak
manupak approved these changes Sep 15, 2021
View reviewed changes
Copy link
Contributor

@manupak manupak left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM.

@tkonolige, it depends on the definition of what we call as a 'library'.
I would still like to think in the world of embedded, the sources in standalone_crt/ is a library of sources to be included to be compiled with embedded compilation tool later.

It would be great to get this in sooner as the usefulness of the standalone wheel is hindered by user having to checkout and build tvm to get access to standalone_crt/ sources, in a deployment scenario.

areusch
areusch approved these changes Sep 15, 2021
View reviewed changes
Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

approved modulo the one comment

@manupak
Copy link
Contributor

manupak commented Sep 15, 2021

@areusch Shall we merge this ?

I think we agree that this step is required eitherway.

@jroesch
Copy link
Member

jroesch commented Sep 15, 2021

@manupa-arm I agree we can land but might be worth revisiting, from my perspective library should only point to .so, .dylib, and so on. I think it would be good to at minimum to clarify in a comment somewhere if not change the code as Tristan suggested.

@jroesch jroesch merged commit 2aebd33 into apache:main Sep 15, 2021
@leandron leandron mentioned this pull request Sep 16, 2021
Merged
AndrewZhaoLuo added a commit to AndrewZhaoLuo/tvm that referenced this pull request Sep 16, 2021
@AndrewZhaoLuo
Merge branch 'main' into aluo/onnx/sceloss
6d3aeb8 
* main: (102 commits)
  Implementation of relay_to_tir target hook (apache#8423)
  [Onnx] Fix NLL Loss tests (apache#8971)
  [Bugfix] Fix other div zero errors also in rewrite_simplify (apache#8983)
  [ONNX] enable the onnx tests after PR apache#8274 merged (apache#9019)
  [Hexagon] Disable `thread_local` on Hexagon (apache#9025)
  [Hexagon] Allow undefined symbols in libtvm_runtime.so on Hexagon (apache#9024)
  [Onnx] Add momentum (apache#9000)
  fix (apache#9021)
  [Community] @AndrewZhaoLuo -> Reviewer (apache#9020)
  [Hexagon] Implement model launcher (apache#8986)
  [Relay][Pass] Add ExtractOperators pass (apache#8996)
  [BYOC][TensorRT] Add TensorRT own int8 calibration support to TensorRT BYOC integration (apache#8808)
  [ONNX] Add Einsum converter (apache#8985)
  Add standalone_crt/ to be part of the wheel package, when available. (apache#9005)
  [Relay] Remove memory planing from LowerTEPass  (apache#8974)
  [Hexagon] Treat floats as float32 when passing args to offloaded kernels (apache#9010)
  [Runtime] Pipeline Executor Initial patch. (apache#8702)
  [Hexagon] `llvm-options` attribute is an array of strings (apache#9011)
  disable cuda int8 schedule for non-cuda gpu target (apache#9014)
  [Torch] Add an option to make imported models compatible with the Relay text parser (apache#9015)
  ...
ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
@leandron @ylc
Add standalone_crt/ to be part of the wheel package, when available. (a…
79feed3 
…pache#9005)

* When using a packaged TVM such as tlcpack, it is impossible to run
  `tvm.micro.get_standalone_crt_dir()`, because the subtree
  `standalone_crt/` is not available.

* This patch adds `standalone_crt/` as `data_files`, so that they
  can be picked up by _ffi.libinfo.find_lib_path() and therefore
  be found when `tvm.micro.get_standalone_crt_dir()` is invoked.
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
@leandron @ylc
Add standalone_crt/ to be part of the wheel package, when available. (a…
7361a73 
…pache#9005)

* When using a packaged TVM such as tlcpack, it is impossible to run
  `tvm.micro.get_standalone_crt_dir()`, because the subtree
  `standalone_crt/` is not available.

* This patch adds `standalone_crt/` as `data_files`, so that they
  can be picked up by _ffi.libinfo.find_lib_path() and therefore
  be found when `tvm.micro.get_standalone_crt_dir()` is invoked.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manupak manupak manupak approved these changes

@tkonolige tkonolige tkonolige approved these changes

@areusch areusch areusch approved these changes

@comaniac comaniac Awaiting requested review from comaniac

@jroesch jroesch Awaiting requested review from jroesch

@junrushao junrushao Awaiting requested review from junrushao

@tqchen tqchen Awaiting requested review from tqchen

@yzhliu yzhliu Awaiting requested review from yzhliu

@zhiics zhiics Awaiting requested review from zhiics

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@leandron @manupak @jroesch @areusch @tkonolige