Consolidates discovery of Prefect server Services to the base class
#16913
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrisguidry
commented
Jan 30, 2025
| self.logger = get_logger(f"server.services.{self.name.lower()}") | ||
|
|
||
|
|
||
| class LoopService(Service, abc.ABC): |
There was a problem hiding this comment.
Note I moved the base LoopService implementation here
chrisguidry
force-pushed
the
consolidate-services-discovery
branch
from
2eece0a to
0d92cca
Compare
chrisguidry
commented
Jan 30, 2025
| ) | ||
|
|
||
|
|
||
| def _known_service_modules() -> list[ModuleType]: |
There was a problem hiding this comment.
This is that spot where we still have to explicitly expose all the service modules (I moved this from the CLI module). This essentially serves as a "registry" of the services
chrisguidry
force-pushed
the
consolidate-services-discovery
branch
from
0d92cca to
72599c9
Compare
chrisguidry
requested review from
cicdw,
desertaxle and
zzstoatzz
as code owners
CodSpeed Performance ReportMerging #16913 will not alter performanceComparing Summary
|
desertaxle
reviewed
Jan 30, 2025
chrisguidry
requested review from
aaazzam,
kevingrismore,
daniel-prefect and
seanpwlms
as code owners
Following on the work in #16898 to unify Prefect's `Service` implementations, this moves the discovery routine from the CLI to the base `Service` implementation. This also includes a couple of additional niceties: - Each service is now responsible for knowing which `Setting` enables it. This eventually means that when adding a new service, we won't need to modify the `api/server.py` lifespan or the CLI. The conditional toggling of services on and off will be handled by asking the service if it should be enabled. - The way service names are handles is more uniform across all the subclasses. I also cleaned up the descriptions of all services to make them more punchy and imperative. - The table of services uses the setting name as part of how it shows whether the service is enabled, which means folks shouldn't have to hunt through the code or docs to find the setting. We do still have an explicit listing of all the known service modules, because I wasn't too keen to go down the rabbithole of automatic discovery from entrypoints or anything like that. Definitely open to ideas on a better place for that. This version doesn't unify how the `api/server.py` lifespan starts services, but that will be coming in a future update.
chrisguidry
force-pushed
the
consolidate-services-discovery
branch
from
4503934 to
2c22c5a
Compare
desertaxle
approved these changes
Jan 30, 2025
aaazzam
approved these changes
Jan 30, 2025
chrisguidry
added a commit
that referenced
this pull request
Jan 31, 2025
In the final step of the plan to unify how services are handled ( after #16898 and #16913), we tie everything together by having both the API lifespan in `prefect server start` and the CLI `prefect server services start` use the same service running logic. At this point, when adding a new service, we should just need to add a new subclass, implement its settings class, and then make sure its module is represented in `server/services/base.py`.
4 tasks
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following on the work in #16898 to unify Prefect's
Serviceimplementations, this moves the discovery routine from the CLI to the
base
Serviceimplementation. This also includes a couple ofadditional niceties:
Each service is now responsible for knowing which
Settingenablesit. This eventually means that when adding a new service, we won't
need to modify the
api/server.pylifespan or the CLI. Theconditional toggling of services on and off will be handled by asking
the service if it should be enabled.
The way service names are handled is more uniform across all the
subclasses. I also cleaned up the descriptions of all services to
make them more punchy and imperative.
The table of services uses the setting name as part of how it shows
whether the service is enabled, which means folks shouldn't have to
hunt through the code or docs to find the setting.
We do still have an explicit listing of all the known service modules,
because I wasn't too keen to go down the rabbithole of automatic
discovery from entrypoints or anything like that. Definitely open to
ideas on a better place for that.
This version doesn't unify how the
api/server.pylifespan startsservices, but that will be coming in a future update.
Checklist
<link to issue>"mint.json.