Feature: Add a project launcher #67 #75
Conversation
There was a problem hiding this comment.
Hi @nikhiljangra264, thanks a lot for this great start! I've also fixed 2 bugs while testing this version and have already pushed them here. For the rest, I've taken a quick look and left some comments.
omeryusufyagci
added
feature
There was a problem hiding this comment.
Thanks for the changes @nikhiljangra264! I've also tested it locally on an Ubuntu system that already had all dependencies installed. Looks good already; please see my comments for details.
config.json
Outdated
| "deep_filter_path": "MediaProcessor/res/deep-filter-0.5.6-x86_64-unknown-linux-musl", | ||
| "deep_filter_tarball_path": "MediaProcessor/res/DeepFilterNet3_ll_onnx.tar.gz", | ||
| "deep_filter_encoder_path": "MediaProcessor/res/DeepFilterNet3_ll_onnx/tmp/export/enc.onnx", | ||
| "deep_filter_decoder_path": "MediaProcessor/res/DeepFilterNet3_ll_onnx/tmp/export/df_dec.onnx", | ||
| "deep_filter_path": "MediaProcessor\\res\\deep-filter-0.5.6-x86_64-unknown-linux-musl", | ||
| "deep_filter_tarball_path": "MediaProcessor\\res\\DeepFilterNet3_ll_onnx.tar.gz", | ||
| "deep_filter_encoder_path": "MediaProcessor\\res\\DeepFilterNet3_ll_onnx\\tmp\\export\\enc.onnx", | ||
| "deep_filter_decoder_path": "MediaProcessor\\res\\DeepFilterNet3_ll_onnx\\tmp\\export\\df_dec.onnx", |
There was a problem hiding this comment.
This may fix it for windows, but breaks it for Unix. While testing this PR on macOS I got some panics from the library which was caused by a file not found due to this. This happened after running the launcher.
Please dynamically update the paths for Windows and let's keep the default paths with the Unix slashes. There seems to be an issue with the dynamic update of this. Could you please check ? Thanks
There was a problem hiding this comment.
The issue is launcher.py successfully updates the path but config.json modified can be accidently pushed with other changes (In this case).
we can change them back after running but what if user runs via command python app.py. That will not work because of paths. So for windows development we have to be careful not to push the config.json.
There was a problem hiding this comment.
Well, the launcher will be the official method to launch any application or even for setting up the repository. Of course it's open source, so everyone is free to hack-away at the code, but we don't necessarily have to support use cases that fall outside of the officially supported methods -that's why we provide a launcher.
I agree with you on your other point though. I'd suggest exploring a runtime override (potentially even a second runtime_config.json) instead of directly overwriting the file. The config.json would serve as a base.
On a second note, we should add a check on app.py to warn the user if it's called from outside the launcher.
There was a problem hiding this comment.
we should add a runtime json that we dont have to warn user about launching outside the launcher.py
we should standardize to use launcher.py
nikhiljangra264
force-pushed
the
main
branch
from
b9869a9 to
92101d0
Compare
1. Capitalize virutal env constants `VENV_NAME` and `VENV_DIR` 2. Use pathlib lib for paths
Add python dependencies and MSYS2 as dependency
Now `config.json` is converted to `runtime_config.json` file for cross paltform compatibility.
1. Fix launcher.py runtime errors 2. utf_16 size error on Windows
1. Move config file handling and logging config to `Config` 2. Move Virtual env handling, running command, check internet connectivity to `Utils` class. 3. Move running web application to `WebApplication` class.
…bute, update sleep time and URL usage.
1. Rename `Config` to `ConfigManager`, required_dependencies to dependencies, virtualenv to venv 2. Add seprate function for updating config for windows
There was a problem hiding this comment.
Thanks @nikhiljangra264, looks all good for the updates since the last review!
Have you tested this on Win10, Darwin, and Linux? I don't have access to my HW to do platform test for a few more days.
Otherwise, this is quite OK for an initial implementation. I'll test it over the weekend, and if all goes well, this should get merged before next week!
Tested on linux, Windows 11, deleted everthing and download again working fine. |
This fixes an issue where the .dylib was pointing at a hardcoded local path instead of @rpath.
There was a problem hiding this comment.
I tested on Darwin macOS.
The lack of any logging threw me off initially. We should tell the user at least the steps the launcher is taking. Not debug level details, but enough to make them understand things are progressing. I thought the launcher was hanging until I started with debug mode.
Then, I ran into an issue with the .dylib pointing to a local hardcoded path instead of @ rpath.
I had seen this the last time around and fixed it, but apparently that was gone with the rebase. Now it's fixed again and I've updated the PR as you'll see.
I'll still take a look at it on Windows, but in the meantime please double check to see we haven't missed anything and take another look at the logging we're providing.
Thanks!
|
Checked the code. |
Thanks, and no worries! Trying to understand why I wasn't seeing any logs, I discovered that the logging setup is delegated to the WebApplication class. I would recommend adding dedicated methods to setup/initialize and cleanup (if needed) the launcher itself. That would include setting up the logging, resolving common dependencies, etc. So, we shouldn't treat it as if WebApp will be the only user of the launcher. |
closes #67
Added launcher.py