Refactor Client's Artifact CRUD methods into Mixin #16496
Conversation
aaazzam
requested review from
cicdw,
desertaxle and
zzstoatzz
as code owners
CodSpeed Performance ReportMerging #16496 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
I have a loosely-held preference to house this somewhere other than the orchestration module because nesting artifacts under orchestration strikes me as unintuitive. I don't have a good alternative, but we'll need a sane organization system to accommodate all the different domain clients.
There was a problem hiding this comment.
yep, makes sense. other clients do directories by namespace. So whatever module holds your client, there's a file per namespace adjacent to it.
There was a problem hiding this comment.
To give ourselves some wiggle room to reorganize later, can you rename this to _artifacts.py to let users know the name and location of this module might change later?
There was a problem hiding this comment.
Sounds good to me!
| def request( | ||
| client: Client, | ||
| method: Literal["GET", "POST", "PUT", "DELETE", "PATCH"], | ||
| path: ServerRoutes, | ||
| path_params: Optional[dict[str, Any]] = None, | ||
| **kwargs: Any, | ||
| ) -> Response: | ||
| if path_params: | ||
| path = path.format(**path_params) # type: ignore | ||
| return client.request(method, path, **kwargs) | ||
|
|
||
|
|
||
| async def arequest( | ||
| client: AsyncClient, | ||
| method: Literal["GET", "POST", "PUT", "DELETE", "PATCH"], | ||
| path: ServerRoutes, | ||
| path_params: Optional[dict[str, Any]] = None, | ||
| **kwargs: Any, | ||
| ) -> Response: | ||
| if path_params: | ||
| path = path.format(**path_params) # type: ignore | ||
| return await client.request(method, path, **kwargs) |
There was a problem hiding this comment.
Could you turn these two functions into base classes from which domain clients can inherit? It feels a bit weird for a domain client to need to pass its 'private' client to another function to make a request. The base class could also include the __init__ that each of the artifact clients you created declares.
There was a problem hiding this comment.
I think this is addressed in:
| class BaseClient: | ||
| def __init__(self, client: "Client"): | ||
| self._client = client | ||
| super().__init__() |
There was a problem hiding this comment.
I don't think this is necessary since BaseClient doesn't have a base class declared.
| super().__init__() |
| class BaseAsyncClient: | ||
| def __init__(self, client: "AsyncClient"): | ||
| self._client = client | ||
| super().__init__() |
There was a problem hiding this comment.
I don't think this is necessary since BaseAsyncClient doesn't have a base class declared.
| super().__init__() |
|
|
||
| from typing import TYPE_CHECKING, Any, Literal, TypeAlias | ||
|
|
||
| from prefect.client.orchestration.routes import ServerRoutes |
There was a problem hiding this comment.
This could be a forward ref too, right?
| "limit": kwargs.get("limit", None), | ||
| "offset": kwargs.get("offset", 0), | ||
| "sort": kwargs.get("sort", None), |
There was a problem hiding this comment.
If the defaults are declared on the typed dicts at the top of this module, do we still need to provide a default value for the .get call here?
Related to #16472, this PR reduces the memory footprint of importing
PrefectClientfrom 34MB to 33.5MB.Artifactshas the smallest footprint of the 12 primitives inPrefectClient, so I expect replicating this strategy should land us something north of a 25% memory reduction when all is said and done. It accomplishes this by:This is a wash on readability IMO - isolating "Artifact CRUD" into a standalone module makes reasoning about the "full client" take one more click, but not having to dig around for artifact-y things and being able to share TypedDict stuff makes up for it.