feat: API pipeline run get has execution summary#97
feat: API pipeline run get has execution summary#97yuechao-qin wants to merge 1 commit intoycq/api_pipeline_run_list_has_endedfrom
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
78c4a89 to
96dfb6c
Compare
ea578e3 to
489c797
Compare
96dfb6c to
034f57b
Compare
489c797 to
3df5bde
Compare
3df5bde to
5014f69
Compare
034f57b to
b25a858
Compare
| self, | ||
| session: orm.Session, | ||
| id: bts.IdType, | ||
| include_execution_stats: bool = False, |
There was a problem hiding this comment.
To each their own on this:
I prefer to use an expand class for these use cases, which is more extensible in the future.
@dataclasses.dataclass(frozen=True)
class PipelineRunResponseExpandOptions:
"""A type-safe structure to define which fields to expand."""
execution_stats: bool = False
then using it as:
if expand.execution_stats:
response = self._populate_execution_stats(session=session, response=response)
There was a problem hiding this comment.
You could totally say "pre-mature class" for just one expansion, but I find the consistency and readiness preferable over future refactors or inconsistency.
| raise ItemNotFoundError(f"Pipeline run {id} not found.") | ||
| return PipelineRunResponse.from_db(pipeline_run) | ||
| response = PipelineRunResponse.from_db(pipeline_run) | ||
| if include_execution_stats: |
There was a problem hiding this comment.
Maybe we should expand the PipelineRun rather than the response? It provides more value to the rest of the codebase that might want to expand this information in other places later on.
This suggestion implies moving the logic to the service layer (e.g. a PipelineRunsService rather than what is currently PipelineRunsApiService_Sql) and adding the field to the PipelineRun model rather than onto the response model.
There was a problem hiding this comment.
PS: This relates to the fact that there might be opportunities to better organize our backend code in general. For example, orchestrator_sql.py has a lot going on.
| next_page_token=next_page_token, | ||
| ) | ||
|
|
||
| def _populate_execution_stats( |
There was a problem hiding this comment.
I would prefer to put general-purpose business logic on a separate class such as a PipelineRunsService rather than having functions like this in a module, and on a class, that is specifically tied to the http transport layer.

TL;DR
Added execution statistics to the pipeline run get endpoint (
GET /api/pipeline_runs/{id}).What changed?
getmethod to optionally include execution statisticsinclude_execution_stats(default: False) to thegetmethod_populate_execution_statsHow to test?
include_execution_stats=TrueTestPipelineRunServiceGetclass to ensure the functionality works as expectedRun the new test cases in
tests/test_api_server_sql.py:Why make this change?
has_endedto the response of PieplineRunsService.list #87tangle-deployneeds to know if a specific pipeline run is completed