feat: API pipeline run get has execution summary

[yuechao-qin]

TL;DR

Added execution statistics to the pipeline run get endpoint (GET /api/pipeline_runs/{id}).

What changed?

• Enhanced the get method to optionally include execution statistics
• Added a new parameter include_execution_stats (default: False) to the get method
• Extracted the execution stats population logic into a new helper method _populate_execution_stats

How to test?

1. Call the pipeline run get endpoint with include_execution_stats=True
2. Verify that the response includes execution status statistics and summary
3. Run the new tests in TestPipelineRunServiceGet class to ensure the functionality works as expected

Run the new test cases in tests/test_api_server_sql.py:

pytest tests/test_api_server_sql.py

Why make this change?

• Related to API Server - Add has_ended to the response of PieplineRunsService.list #87
• tangle-deploy needs to know if a specific pipeline run is completed

[yuechao-qin]

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.
Learn more

• feat: GetExecutionNodes API returns number of total and ended nodes #99
• test: Add tests for GetGraphExecution API #98
• feat: API pipeline run get has execution summary #97 👈 (View in Graphite)
• feat: API pipeline run list has execution summary #96
• refactor: Function to get execution status stats #95
• feat: Model to track node states and has_ended flag #94
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

[morgan-wowk]

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)

[morgan-wowk]

You could totally say "pre-mature class" for just one expansion, but I find the consistency and readiness preferable over future refactors or inconsistency.

[yuechao-qin]

I agree, I like this pattern too, I tend to do this when there are > 5 params.

[morgan-wowk]

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.

[morgan-wowk]

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.

[yuechao-qin]

I think that's a good idea. Happy to do that in a separate PR, I'm hoping to keep the scope minimal for this PR.

[morgan-wowk]

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.

[yuechao-qin]

I agree more organization is definitely needed in this file. I have another PR https://app.graphite.com/github/pr/TangleML/tangle/199, that's helping doing some of this refactoring.

TLDR: There is a new /execution/* folder that this stack of PR logic could fit nicely in. I'm just waiting on landing that and then can do cleanup in a future PR.