Store process information and manage application logging#94
Store process information and manage application logging#94
Conversation
and pass file descriptor to launched process to get output
|
I've not set it as Draft even though it has some issues on other platforms. It works in usable state on Windows and it is good to have some testing feedback while working on other platforms issues. |
…into enhancement/store-process-info
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR adds process information storage and management capabilities to AYON applications. It introduces a local SQLite database to track launched processes and provides a Process Monitor UI for viewing and managing these processes.
Key changes:
- Store launched process information in a local SQLite database with process details like PID, output file paths, and status
- Add a Process Monitor GUI tool accessible from the tray menu for viewing process status and output logs
- Implement process status checking using psutil with fallbacks to system tools
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| client/pyproject.toml | Adds psutil dependency for process status checking |
| client/ayon_applications/ui/process_monitor.py | New Process Monitor GUI with table view, output display, and cleanup functionality |
| client/ayon_applications/manager.py | Adds ProcessInfo dataclass and database operations for storing/retrieving process information |
| client/ayon_applications/addon.py | Integrates Process Monitor as a tray action in the AYON interface |
…into enhancement/store-process-info
|
|
||
|
|
||
| @dataclass | ||
| class ProcessInfo: |
There was a problem hiding this comment.
Should have id or process_id with unique value. There is no column with a unique value in the database at this moment...
I would expect you can see output of already closed applications, in that case the hash don't have to work, and calculation of the hash is unnecessary slowdown, compared to explicit simple id (especially if this is considered to be on server -> I guess that based on the site id).
There was a problem hiding this comment.
It would also simplified finding the process object to avoid using hash of values that don't have to be unique (even if are combined from multiple columns).
There was a problem hiding this comment.
there is a hash as primary key. It is sha256 of process name, pid, executable and start time.
There was a problem hiding this comment.
And I don't understand why that was used over using uuid? The hash is potential danger from my perspective (especially when it won't be used only for local site database file). You also have to calculate it all the time you want to get information about the process.
| cwd: str | ||
| pid: Optional[int] = None | ||
| active: bool = False | ||
| output: Optional[Path] = None |
There was a problem hiding this comment.
Is this the content of stdout/stderr files? Shouldn't that be loaded in a separate way (so you don't have to open the large files to list processes).
There was a problem hiding this comment.
It is not a content, it is a path. And it is loaded in a separate way in the Manger tool.
There was a problem hiding this comment.
Then the attribute name is not clear and confusing, should be output_path.
There was a problem hiding this comment.
output is typed as Path. Adding it to variable name is IMO redundant in most modern IDEs. In the same line it should be executable_path or start_timestamp_utc. I would agree if it was plain str
|
Why were all my comments resolved? I don't agree the way the processes are stored I'm strongly against using I don't understand why we do store site id to the data, there is no reason to because the database file is stored to site and user specific location, if that's because it will be stored to server "some time" then using |
I forgot to push the changes
Hash is unique enough. I don't see how you could achieve conflicting hashes when you don't create milions of processes within second.
That was removed. |
…into enhancement/store-process-info
and small linting changes
Changelog Description
Store launched process information in a local database and add process monitoring/output manager.
Additional review information
When process is run, this will store information about it in local sqlite3 database that can be later on used by other parts of AYON. It also passes temporary file as stdout and stderr handle, storing its path along the process information.
All this information can be used to read out what the launched process is outputting in situations where it doesn't open its own console. For this Process Monitoring tool is added:
This tool can be run using
Process Monitoritem in Tray menu.What it does:
Further enhancements:
Shell scripts and passing of PID back to AYON
When launching applications through shell scripts on Linux, the process ID returned by the
app_launcher(mid-process) corresponds to the shell process, not the actual application process that the shell script launches.Architecture Overview
manager.py) creates launch contexts and manages application processesapp_launcher.py/app_launcher.cppin ayon-launcher) is the mid-process that handles detached process launching on LinuxSolution
The solution implements a PID file convention that allows shell scripts to communicate the actual application PID back to AYON through the app_launcher mid-process.
How it Works
AYON_PID_FILEenvironment variableIntegration Requirements
For shell scripts to take advantage of this feature:
$AYON_PID_FILEenvironment variable exists&$!(bash) or equivalent$AYON_PID_FILEBash Example
Warning
For retrieving process status, it is using
psutiltool that you need in your dependencies. That means building new dependency package. You can addpsutilmanually but keep in mind that it is binary dependency and thus it will work only with compatible Python host (if you build it for python 3.9 it is unlikely that it will also work in 3.11).Testing notes:
You should be able to run any DCC as usual and see its output in Process Manager. It works on Linux and macos. Try to wrap and execute some application in shell script as shown in the example above - Process Manager should still pick the correct process id and monitor the process. Removing of selected item/All inactive items should work too. If you use ANSI sequences to colorize your shell script, you should see that also in the output (it supports even bold, underline, ... styles).