server : add pidfile option #14242
server : add pidfile option #14242
Conversation
ericcurtin
requested review from
Copilot and
ggerganov
and removed request for
Copilot
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ericcurtin
force-pushed
the
add-pidfile
branch
5 times, most recently
from
e2b5343 to
5c68d23
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a PID file option for the server process so that the process ID can be tracked.
- Implements a new PidFile class in tools/server/server.cpp to handle creation, management, and cleanup of a PID file.
- Adds a new command-line option "--pidfile" to set the PID file path (common/arg.cpp and common/common.h).
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/server/server.cpp | Introduces PidFile class and PID file lifecycle management. |
| common/common.h | Adds a new pidfile field to common_params. |
| common/arg.cpp | Adds a new command-line argument for configuring the pidfile. |
Comments suppressed due to low confidence (1)
tools/server/server.cpp:3780
- [nitpick] Consider renaming the variable 'f' to a more descriptive name such as 'pidFileHandler' to improve code clarity.
PidFile f;
|
@ngxson @ggerganov might be worth another look, it's more advanced now |
| PidFile f; | ||
| if (!params.pidfile.empty()) { |
There was a problem hiding this comment.
the variable is not used outside its scope, so it's better to move inside the if {..}
| PidFile f; | |
| if (!params.pidfile.empty()) { | |
| if (!params.pidfile.empty()) { | |
| PidFile f; |
There was a problem hiding this comment.
No there's a reason it's in the outer scope, we want the destructor to attempt to delete the file when main() exits, not any sooner
There was a problem hiding this comment.
hmm ok maybe put this as a comment for visibility
| } | ||
| }; | ||
|
|
||
| static bool is_old_pid_alive(const std::string & filename) { |
There was a problem hiding this comment.
I think we can move all static function into the class PidFile
| return false; // Process does not exist or other error | ||
| } | ||
|
|
||
| class PidFile { |
There was a problem hiding this comment.
maybe move it to utils.hpp to avoid pollute server.cpp with non-server code
| bool rm = false; | ||
|
|
||
| FILE * open(const std::string & filename, const char * mode, const bool r = false) { | ||
| file = ggml_fopen(filename.c_str(), mode); |
There was a problem hiding this comment.
we already had many functions to handle file i/o in common.cpp, should reuse it instead of inventing a new one
ericcurtin
force-pushed
the
add-pidfile
branch
2 times, most recently
from
7e9ce12 to
57507f3
Compare
So we can track the pid of this process Signed-off-by: Eric Curtin <[email protected]>
So we can track the pid of this process