Refactor emulator start and stop functions for clarity and efficiency #22861
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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>
…to Cjian/check_emulator_running
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
snnn
added
the
platform:mobile
tools/python/util/android/android.py
Outdated
| # 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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This should be only for emulator. Generated by ChatGPT.
There was a problem hiding this comment.
I got
ort-andriod
OK
when I run this command locally.
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>
tools/python/util/android/android.py
Outdated
| @@ -118,10 +126,12 @@ def start_emulator( | |||
| "America/Los_Angeles", | |||
| "-no-snapstorage", | |||
| "-no-audio", | |||
| "-no-snapshot", # No bluetooth | |||
There was a problem hiding this comment.
I thought this parameter was about fast startup from previous run.
There was a problem hiding this comment.
Yeah, that is correct. This is not needed for now.
tools/python/util/android/android.py
Outdated
| 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: |
There was a problem hiding this comment.
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.
| _log.debug("No emulators running.") | ||
| return False # No emulators running | ||
|
|
||
| # Step 2: Check each running emulator's AVD name |
There was a problem hiding this comment.
Should we have step 3 of checking getprop sys.boot_completed like we do in start_emulator to make sure they have successfully started?
There was a problem hiding this comment.
I don't think it is necessary, because It is used by start_emulator which calls this:
- at the beginning of the function to check if the emulator with the avd name is already running.
- at the end of the function to check if the emulator with the avd is running AFTER they have successfully started.
There was a problem hiding this comment.
We should also update the versions in onnxruntime-extensions and onnxruntime-genai once the changes are checked in.
Co-authored-by: Scott McKay <[email protected]>
Co-authored-by: Scott McKay <[email protected]>
Description
This pull request introduces several enhancements and new functionalities to the
tools/python/util/android/android.pyfile, focusing on improving the management of Android emulators. The most important changes include adding a timeout parameter to thestart_emulatorfunction, adding checks to prevent multiple emulators from running simultaneously, and introducing new utility functions to manage emulator processes more effectively.Enhancements to
start_emulatorfunction:timeout_minutesparameter to thestart_emulatorfunction to make the startup timeout configurable. [1] [2]-verbosefor better control and debugging.New utility functions for managing emulator processes:
check_emulator_running_using_avd_name,check_emulator_running_using_process, andcheck_emulator_running_using_pidto check if an emulator is running based on AVD name, process instance, or PID, respectively.stop_emulator_by_procandstop_emulator_by_pidfunctions to stop the emulator process using asubprocess.Popeninstance or PID, with a configurable timeout.stop_emulatorfunction 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