fix(assets): update mllm_role image to reflect recent changes#548
fix(assets): update mllm_role image to reflect recent changes#548UbiquitousLearning merged 2 commits intoUbiquitousLearning:mainfrom
Conversation
WalkthroughGitHub Actions workflow triggers are updated from branch v2 to main. Deployment destination paths in Android remote execution scripts are changed from Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
assets/mllm_role.pngis excluded by!**/*.png
📒 Files selected for processing (7)
.github/workflows/build-android.yml(3 hunks).github/workflows/build-osx.yml(3 hunks).github/workflows/docs-deploy.yml(1 hunks).github/workflows/pymllm-macos-nightly.yml(1 hunks)algorithms/lazy_vlm/run_remote_android.py(1 hunks)playground/cache_reduced_computation/run_remote_android.py(1 hunks)tasks/adb_push.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
algorithms/lazy_vlm/run_remote_android.py (1)
pymllm/utils/adb.py (3)
push_file(257-263)get_shell_context(206-228)execute(79-116)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
playground/cache_reduced_computation/run_remote_android.py (1)
36-36: Verify the target directory exists on the device.The path update is consistent with the PR's migration from
mllm-v2tomllm. Ensure the new destination directory/data/local/tmp/mllm/bin/playground/exists on target Android devices before deployment..github/workflows/pymllm-macos-nightly.yml (1)
6-6: LGTM! Branch trigger updated consistently.The workflow now triggers on the
mainbranch for both pull requests and pushes, aligning with the repository's branch migration.Also applies to: 12-12
.github/workflows/build-osx.yml (1)
6-6: LGTM! Branch triggers updated consistently.The workflow now monitors the
mainbranch for both push and pull request events, aligning with the repository migration.Also applies to: 15-15
.github/workflows/docs-deploy.yml (1)
6-6: LGTM! Documentation workflow updated to main branch.The deployment workflow now triggers on the
mainbranch, consistent with the repository-wide branch migration.Also applies to: 12-12
.github/workflows/build-android.yml (1)
6-6: LGTM! Branch triggers updated consistently.The Android NDK compilation workflow now triggers on the
mainbranch for both push and pull request events.Also applies to: 15-15
algorithms/lazy_vlm/run_remote_android.py (2)
36-36: LGTM! Push destinations updated consistently.All four binary push destinations have been updated from
/data/local/tmp/mllm-v2/bin/algorithms/to/data/local/tmp/mllm/bin/algorithms/, aligning with the directory structure migration.Also applies to: 40-40, 44-44, 48-48
54-54: Path updates are correct for when this code becomes reachable.The LD_LIBRARY_PATH and command-line argument paths have been updated consistently to use
/data/local/tmp/mllm/instead of/data/local/tmp/mllm-v2/. Once theexit(0)on line 50 is addressed, these changes will properly reflect the new directory structure.Also applies to: 59-59
tasks/adb_push.yaml (1)
16-16: Path migration verified as complete across all dependent configurations.The codebase shows zero remaining references to the old
mllm-v2path. All dependent configurations that reference the destination path—specifically LD_LIBRARY_PATH exports intask.py,algorithms/lazy_vlm/run_remote_android.py, andalgorithms/fancy_algorithm/run.py—are correctly updated to/data/local/tmp/mllm/bin/. No additional changes needed.
| exit(0) | ||
| with adb.get_shell_context() as shell: | ||
| shell.execute("cd /data/local/tmp/mllm/bin/algorithms/") | ||
| shell.execute( | ||
| "export LD_LIBRARY_PATH=/data/local/tmp/mllm-v2/bin:$LD_LIBRARY_PATH" | ||
| "export LD_LIBRARY_PATH=/data/local/tmp/mllm/bin:$LD_LIBRARY_PATH" | ||
| ) | ||
| # If Using QNN, uncomment the following line | ||
| # shell.execute("export ADSP_LIBRARY_PATH=/data/local/tmp/mllm/lib64") | ||
| res = shell.execute( | ||
| './lazy_vlm -m /data/local/tmp/mllm-v2/models/Qwen2.5-VL-3B-Instruct/w4a32.mllm -mv v2 -t /data/local/tmp/mllm-v2/models/Qwen2.5-VL-3B-Instruct/tokenizer.json -c /data/local/tmp/mllm-v2/models/Qwen2.5-VL-3B-Instruct/config_w4a32.json -p "Describe this image" -i /data/local/tmp/mllm-v2/bin/gafei.jpeg -s normal' | ||
| './lazy_vlm -m /data/local/tmp/mllm/models/Qwen2.5-VL-3B-Instruct/w4a32.mllm -mv v2 -t /data/local/tmp/mllm/models/Qwen2.5-VL-3B-Instruct/tokenizer.json -c /data/local/tmp/mllm/models/Qwen2.5-VL-3B-Instruct/config_w4a32.json -p "Describe this image" -i /data/local/tmp/mllm/bin/gafei.jpeg -s normal' | ||
| ) | ||
| print(res) |
There was a problem hiding this comment.
Unreachable code: Shell execution commands will not run.
The exit(0) on line 50 prevents the shell context and execution commands (lines 51-61) from ever running. While the path updates in this section are correct, they won't take effect until the exit(0) is removed or relocated.
This appears to be pre-existing code rather than introduced by this PR, but it's worth addressing to make the updated paths functional.
Would you like me to suggest how to properly enable this execution flow, or should this be tracked separately?
🤖 Prompt for AI Agents
In algorithms/lazy_vlm/run_remote_android.py around lines 50 to 61, the early
call to exit(0) makes the subsequent adb shell context and command executions
unreachable; to fix this, remove or move the exit(0) so the with
adb.get_shell_context() block executes (or guard the exit behind a conditional
so it only runs in a path that should terminate), ensuring the environment
exports and lazy_vlm invocation run as intended and outputs are printed.
UbiquitousLearning
merged commit 836aa97
into
UbiquitousLearning:main
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.