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

Refactor emulator start and stop functions for clarity and efficiency #22861

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

jchen351
Copy link
Contributor

@jchen351 jchen351 commented Nov 16, 2024

Description

This pull request introduces several enhancements and new functionalities to the tools/python/util/android/android.py file, focusing on improving the management of Android emulators. The most important changes include adding a timeout parameter to the start_emulator function, adding checks to prevent multiple emulators from running simultaneously, and introducing new utility functions to manage emulator processes more effectively.

Enhancements to start_emulator function:

  • Added a timeout_minutes parameter to the start_emulator function to make the startup timeout configurable. [1] [2]
  • Added a check to prevent starting a new emulator if one with the same AVD name is already running.
  • Included additional emulator arguments -verbose for better control and debugging.
  • Added a final verification step to ensure the emulator has started successfully.

New utility functions for managing emulator processes:

  • Introduced check_emulator_running_using_avd_name , check_emulator_running_using_process, and check_emulator_running_using_pid to check if an emulator is running based on AVD name, process instance, or PID, respectively.
  • Added stop_emulator_by_proc and stop_emulator_by_pid functions to stop the emulator process using a subprocess.Popen instance or PID, with a configurable timeout.
  • Updated the stop_emulator function to use the new utility functions for stopping the emulator process.

These changes enhance the robustness and flexibility of the emulator management utilities, making it easier to handle different scenarios in CI environments and development workflows.

Motivation and Context

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You can commit the suggested changes from lintrunner.

tools/python/util/android/android.py Outdated Show resolved Hide resolved
tools/python/util/android/android.py Outdated Show resolved Hide resolved
tools/python/util/android/android.py Outdated Show resolved Hide resolved
tools/python/util/android/android.py Outdated Show resolved Hide resolved
tools/python/util/android/android.py Outdated Show resolved Hide resolved
tools/python/util/android/android.py Outdated Show resolved Hide resolved
jchen351 and others added 9 commits November 15, 2024 16:58
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You can commit the suggested changes from lintrunner.

tools/python/util/android/android.py Outdated Show resolved Hide resolved
tools/python/util/android/android.py Outdated Show resolved Hide resolved
tools/python/util/android/android.py Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You can commit the suggested changes from lintrunner.

tools/python/util/android/android.py Outdated Show resolved Hide resolved
@snnn snnn added the platform:mobile issues related to ONNX Runtime mobile; typically submitted using template label Nov 16, 2024
# Step 2: Check each running emulator's AVD name
for emulator in running_emulators:
try:
avd_info = subprocess.check_output(["adb", "-s", emulator, "emu", "avd", "name"], text=True).strip()
Copy link
Contributor

@skottmckay skottmckay Nov 18, 2024

Choose a reason for hiding this comment

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

What's the expected output of this command? I tried with both the phones connected to the mac mini but got no output so wondering if it differs for a real device vs an emulator. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be only for emulator. Generated by ChatGPT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got

ort-andriod
OK

when I run this command locally.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You can commit the suggested changes from lintrunner.

tools/python/util/android/android.py Outdated Show resolved Hide resolved
jchen351 and others added 3 commits November 18, 2024 10:30
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You can commit the suggested changes from lintrunner.

tools/python/util/android/android.py Outdated Show resolved Hide resolved
jchen351 and others added 2 commits November 18, 2024 13:56
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@@ -118,10 +126,12 @@ def start_emulator(
"America/Los_Angeles",
"-no-snapstorage",
"-no-audio",
"-no-snapshot", # No bluetooth
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this parameter was about fast startup from previous run.

https://developer.android.com/studio/run/emulator-snapshots

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that is correct. This is not needed for now.

tools/python/util/android/android.py Show resolved Hide resolved
return emulator_process


def stop_emulator(emulator_proc_or_pid: typing.Union[subprocess.Popen, int]):
def is_emulator_running_by_avd(avd_name: str) -> bool:
Copy link
Contributor

@skottmckay skottmckay Nov 21, 2024

Choose a reason for hiding this comment

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

Could we call these something like check_emulator_running_using_avd_name/check_emulator_running_using_process/check_emulator_running_using_pid?

The 'by' sounds like we could be checked how it was started rather than if it's currently running.
#Closed

tools/python/util/android/android.py Show resolved Hide resolved
tools/python/util/android/android.py Show resolved Hide resolved
tools/python/util/android/android.py Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also update the versions in onnxruntime-extensions and onnxruntime-genai once the changes are checked in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform:mobile issues related to ONNX Runtime mobile; typically submitted using template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants