-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add prefect server services
CLI commands
#16706
Conversation
c13ccb8
to
93c6d0a
Compare
CodSpeed Performance ReportMerging #16706 will not alter performanceComparing Summary
|
292f50b
to
14ad097
Compare
b71f4be
to
37dff5c
Compare
de0efce
to
fbd11a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few small comments, overall the implementation makes sense to me - I'll probably give it another pass in a bit
src/prefect/cli/server.py
Outdated
), | ||
): | ||
"""Start all enabled Prefect services in one process.""" | ||
pid_file = Path(PREFECT_HOME.value()) / "services" / "manager.pid" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two minor points:
- given this is an important file and a global, I suggest initializing it (but not creating it) at the top of the file and calling it
PID_FILE
- to mirror the
server start
command, I think it might be more consistent to usePath(PREFECT_HOME.value()) / "services.pid"
rather than creating a new subdirectory
clean up fix table print
restore useful comments
903f5df
to
0bbd572
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Propose to downgrade many of the new logs to DEBUG
: I think anything that displays "implementation" information like service names is only useful when trying to understand what's going on under the hood, hence DEBUG
vs. INFO
which I think is broad strokes useful information relevant to all users ("services all started", etc.).
Co-authored-by: Chris White <[email protected]>
Co-authored-by: Chris White <[email protected]>
Co-authored-by: Chris White <[email protected]>
Co-authored-by: Chris White <[email protected]>
Co-authored-by: Chris White <[email protected]>
Co-authored-by: Chris White <[email protected]>
Co-authored-by: Chris White <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
wondering if maybe should be
experimental
as I think we may want to add some logging config and maybe update the signature of things - feedback?start
: start all loop services in the foreground--background
start them in the backgroundls
: list all known loop services and whether they are enabledstop
: stop any running loop servicesboth
start
andstop
should be idempotentin order to ensure the
--background
option can be managed by a single process, I created a hiddenmanager
command underservices
(which does not show up in--help
). open to other suggestionsdemo usage