Skip to content

chore: API Server - Clean up get_graph_execution_state (deprecated Row.tuple(), dead commented-out query) #364

Description

@AryanKansagara

Summary

ExecutionNodesApiService_Sql.get_graph_execution_state in cloud_pipelines_backend/api_server_sql.py has a couple of small bits of leftover debt. Neither changes behaviour today, but one of them will break on the next SQLAlchemy major release. I'd like to clean both up in one small PR if that's welcome.

1. Deprecated Row.tuple() call

Line 758 (at c0cc194):

child_execution_id, status, count = row.tuple()

Row.tuple() has been deprecated since SQLAlchemy 2.0.19 in favour of Row._tuple(). The project pins sqlalchemy>=2.0.51, where the old name still works but emits an SADeprecationWarning on every call. It's slated for removal in SQLAlchemy 2.1, at which point /api/executions/{id}/state and /api/executions/{id}/graph_execution_state would fail for any execution that has children.

The TODO right above the call (lines 753 to 754) already asks for this rename. It's the only .tuple() call in the repo.

To reproduce:

pytest tests -q -W error::DeprecationWarning

This fails TestGetGraphExecutionState::test_direct_container_children and TestGetGraphExecutionState::test_three_level_mixed_stats with:

sqlalchemy.exc.SADeprecationWarning: The Row.tuple() method is deprecated in favor of Row._tuple() ... (deprecated since: 2.0.19)

2. Commented-out alternative query

Lines 675 to 703 are a fully commented-out version of child_descendants_query that joins ContainerExecution, with a note explaining it can't be used because ContainerExecution rows don't exist for nodes that haven't started yet. The live query below it is the working replacement. The block is 29 lines and hasn't changed since the function was first written.

Proposed change

  • Replace row.tuple() with row._tuple() and drop the first TODO. Keep the second TODO about SQLAlchemy 2.1, since it's still accurate.
  • Delete the commented-out query. If the explanation is worth keeping, I'd shrink it to a one-line comment above the live query.
  • No behaviour change. The existing tests in tests/test_execution_nodes_api_service.py::TestGetGraphExecutionState cover the empty, direct-children, and three-level cases, and the full suite passes with -W error::DeprecationWarning after the change.

Question

Would you take this as a PR? Happy to limit it to just the _tuple() rename if you'd rather keep the commented-out query around for reference.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions