-
Notifications
You must be signed in to change notification settings - Fork 158
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
[onert] Revisits python API #13583
[onert] Revisits python API #13583
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this idea of renaming! It's making the API so much cleaner from user perspective.
Few small remarks in later comments. More of a clean up than anything.
if operations: | ||
session = nnfw_session(nnpackage_path, backends, operations) | ||
session = nnfw_onert.onert.session(nnpackage_path, backends, operations) | ||
else: | ||
session = nnfw_session(nnpackage_path, backends) | ||
session = nnfw_onert.onert.session(nnpackage_path, backends) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to point out the overload for onert.session(nnpackage_path, backends, operations)
has been removed in #13468 Could it be removed in this PR along other changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I'll remove
@@ -7,9 +7,9 @@ def main(nnpackage_path, backends="cpu", operations=""): | |||
# operations is optional to assign a specific backends to each operation. | |||
# The default value of backends is "cpu". | |||
if operations: | |||
session = nnfw_session(nnpackage_path, backends, operations) | |||
session = onert.session(nnpackage_path, backends, operations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
session = onert.session(nnpackage_path, backends, operations) |
Same as previous comment ref: #13468 Could it be removed in this PR along other changes?
infra/nnfw/python/README.md
Outdated
``` | ||
|
||
This definition has to be set at the top of the script using nnfw python API. | ||
|
||
``` | ||
from nnfwapi.libnnfw_api_pybind import * | ||
import nnfw_onert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import nnfw_onert | |
from nnfw_onert import onert |
Wouldn't it be more inline with actual usage? As far as I understand nnfw_onert
is just a namespace package, so pointing out to shorter path would be even better, just to use onert.something
later on.
|
bd5e48f
to
da4f789
Compare
@Samsung/one_onert PTAL |
da4f789
to
89cb1ba
Compare
- Rename python package: nnfwapi -> onert - Native library install path: "nnfwapi/onert" -> "nnfw_onert/natvie" - Rename binding script: "libnnfw_api_pybind.py" -> "infer.py" - Rename binding session class: "nnfw_session" -> "session" - Update documentation - Update example ONE-DCO-1.0-Signed-off-by: Hyeongseok Oh <[email protected]>
89cb1ba
to
0ffee05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ONE-DCO-1.0-Signed-off-by: Hyeongseok Oh [email protected]