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

[onert/python] Separate prepare from session constructor #14525

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

ragmani
Copy link
Contributor

@ragmani ragmani commented Jan 6, 2025

This commit separates prepare function from initializing session instance.

ONE-DCO-1.0-Signed-off-by: ragmani [email protected]

@ragmani ragmani requested a review from a team January 6, 2025 07:41
@ragmani
Copy link
Contributor Author

ragmani commented Jan 6, 2025

For #14505
Draft : #14492

@ragmani ragmani added the PR/ready for review It is ready to review. Please review it. label Jan 6, 2025
@hseok-oh
Copy link
Contributor

hseok-oh commented Jan 6, 2025

Isn't it better to introduce new python API for prepare function to split with __init__ function?

@ragmani
Copy link
Contributor Author

ragmani commented Jan 6, 2025

Isn't it better to introduce new python API for prepare function to split with init function?

I also think it's better, but it makes the python API changes for inference. Is it OK?
From:

    # Create session and load nnpackage
    # The default value of backends is "cpu".
    session = infer.session(nnpackage_path, backends)
    ...
    outputs = session.inference()

To:

    # Create session and load nnpackage
    # The default value of backends is "cpu".
    session = infer.session(nnpackage_path, backends)
    session = infer.prepare()
    ...
    outputs = session.inference()

@hseok-oh
Copy link
Contributor

hseok-oh commented Jan 6, 2025

Considering easy usage, we can remain current API - prepare at once if session is created with file path, and support new feature to create session only when file path is not passed and support load API and prepare API separately.

@hseok-oh
Copy link
Contributor

hseok-oh commented Jan 6, 2025

@ragmani Please resolve conflict

@ragmani
Copy link
Contributor Author

ragmani commented Jan 6, 2025

Considering easy usage, we can remain current API - prepare at once if session is created with file path, and support new feature to create session only when file path is not passed and support load API and prepare API separately.

I added the code to allow the use of Prepare in the draft 8ef5277. If you feel it's acceptable, I will apply it to the next PR.

@ragmani ragmani force-pushed the onert/separate_prepare_session branch from 34e5d68 to fa432a6 Compare January 6, 2025 12:05
@ragmani ragmani changed the title [onert/python] Separate prepare from session.init [onert/python] Separate prepare from session constructor Jan 6, 2025
This commit separates prepare function from initializing session instance.

ONE-DCO-1.0-Signed-off-by: ragmani <[email protected]>
@ragmani ragmani force-pushed the onert/separate_prepare_session branch from fa432a6 to 128961c Compare January 6, 2025 12:09
Copy link
Contributor

@ys44kim ys44kim left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@zetwhite zetwhite left a comment

Choose a reason for hiding this comment

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

LGTM

@zetwhite zetwhite requested a review from a team January 7, 2025 01:56
Copy link
Contributor

@hseok-oh hseok-oh left a comment

Choose a reason for hiding this comment

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

LGTM

@hseok-oh hseok-oh merged commit d978911 into Samsung:master Jan 7, 2025
9 checks passed
@ragmani ragmani deleted the onert/separate_prepare_session branch January 7, 2025 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/ready for review It is ready to review. Please review it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants