You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/04/05 21:03:33 UTC
[GitHub] [tvm] Lunderberg opened a new pull request, #10907: [Hexagon] Move aot/graph_executor interactions into launcher
Lunderberg opened a new pull request, #10907:
URL: https://github.com/apache/tvm/pull/10907
Follow-up from https://github.com/apache/tvm/pull/10581, applying similar changes to the AOT and graph executor interactions. This moves the file management and upload/download from the unit tests into the launcher.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #10907: [Hexagon] Move aot/graph_executor interactions into launcher
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #10907:
URL: https://github.com/apache/tvm/pull/10907#issuecomment-1092913946
> We could have a utility function somewhere in tvm/contrib/hexagon that does the export_library together with the link-step workaround. That should save some work from the tests using AoT.
Good point, and I like the idea of centralizing the link-step workaround, so it will only need to be removed from one spot once it is not longer needed. I was thinking that it would make sense to have it directly in `tvm.contrib.hexagon.create_aot_shared`, since right now it is necessary for all invocations.
The utility function would get a lot of the same benefits. I had been hoping to also avoid requiring functionality tests to handle file management and cleanup, as the file management isn't relevant to the behavior being tested. I'm open to having a testing-specific utility function, though I'd still prefer having it be part of the session object, as that would allow for additional ergonomic benefits in the future (e.g. Uploads triggered through the session having a session-specific prefix to avoid name collisions, and being cleaned up after the session.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on pull request #10907: [Hexagon] Move aot/graph_executor interactions into launcher
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on PR #10907:
URL: https://github.com/apache/tvm/pull/10907#issuecomment-1091951093
IMO there are two types of interactions with a device: ones that have to do with setting up the device, and ones that have to do with running code on it. The first group would be, for example, uploading files to it, starting servers, etc, while the second would be loading modules into memory, creating arrays in memory, invoking functions, etc. The first set would be the responsibility of the launcher, while the second would belong to the session.
In this view, creating executors would belong to the session, so your changes are ok in that sense. Still, I'd like to make the division of responsibilities clear (not necessarily in this PR though).
Finally, I don't like the idea of passing executor factories to the launcher/session code. Executor factories are not persistent objects. What's persistent (i.e. can be saved/loaded) are the actual modules. We should operate in terms of those instead. In particular, the `export_library` stuff does not belong in either launcher or session.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #10907: [Hexagon] Move aot/graph_executor interactions into launcher
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #10907:
URL: https://github.com/apache/tvm/pull/10907#issuecomment-1090376360
For the re-use, each unit test is run with an independent hexagon session. The [`hexagon_session` test fixture](https://github.com/apache/tvm/blob/main/tests/python/contrib/test_hexagon/conftest.py#L188) is a function-scope fixture, so it gets regenerated before every test (or parametrization of a test), and every test runs with a unique session.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic merged pull request #10907: [Hexagon] Move aot/graph_executor interactions into launcher
Posted by GitBox <gi...@apache.org>.
kparzysz-quic merged PR #10907:
URL: https://github.com/apache/tvm/pull/10907
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on pull request #10907: [Hexagon] Move aot/graph_executor interactions into launcher
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on PR #10907:
URL: https://github.com/apache/tvm/pull/10907#issuecomment-1093108741
Could we have separate function inside `Session` that takes the factory objects (something like `get_executor_from_factory` or something like that)? Then the other functions would take a module, or module+json.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on pull request #10907: [Hexagon] Move aot/graph_executor interactions into launcher
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on PR #10907:
URL: https://github.com/apache/tvm/pull/10907#issuecomment-1091984578
We could have a utility function somewhere in tvm/contrib/hexagon that does the `export_library` together with the link-step workaround. That should save some work from the tests using AoT.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #10907: [Hexagon] Move aot/graph_executor interactions into launcher
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #10907:
URL: https://github.com/apache/tvm/pull/10907#issuecomment-1090410339
For moving the implementation from the session to the launcher, is that primarily to allow them to be overloaded by the `HexagonLauncherAndroid` and `HexagonLauncherSimulator` subclasses? Otherwise, I'm not quite following how moving the implementation from `Session` to `HexagonLauncherRPC` would avoid the re-use of the session object.
I had been thinking of the session as the primary way that a real or simulated device is exposed to the user, since it holds the `session.device`, and wanted to avoid requiring test code to interact with both a launcher and a session generated from that launcher. Since the session is always generated from a launcher, the session can track which launcher it was generated from, and delegate any functionality that is needed at that point. That said, whichever has the full implementation should be called by the other. On a re-read, I realized that I didn't `HexagonLauncherRPC.get_graph_executor` and `HexagonLauncherRPC.get_aot_executor` functions should be updated to directly call the `Session` functions, so the two have different accepted argument types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on pull request #10907: [Hexagon] Move aot/graph_executor interactions into launcher
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on PR #10907:
URL: https://github.com/apache/tvm/pull/10907#issuecomment-1090487975
Let me think about it for a bit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #10907: [Hexagon] Move aot/graph_executor interactions into launcher
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #10907:
URL: https://github.com/apache/tvm/pull/10907#issuecomment-1095130355
Sure. I like that division, because it avoids a lot of the argument checks that are necessary if both behaviors are in a single function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #10907: [Hexagon] Move aot/graph_executor interactions into launcher
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #10907:
URL: https://github.com/apache/tvm/pull/10907#issuecomment-1092904174
I like the general division between launcher and session, and that mirrors the general compile-time / run-time split that exists throughout TVM.
From what I can tell, there's two distinct use-cases that are pointing toward the different preferred designs. In a production environment repeatedly running the same model, the file copying should only be done once, with each session loading the model that has already been uploaded. In this use case, the saved binary file is fundamental; created by the user, uploaded by the launcher, and loaded by the session. In a testing environment where each model is different, the file copying must be done prior to each session. In this use case, the saved binary file is a temporary intermediate, whose entire purpose is replicate the local built module onto the remote session.
I think passing the executor factories makes sense to support the second use case, because those are the objects that have sufficient information to fully describe the state being replicated onto the remote session.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org