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.
Summary
ExecutionNodesApiService_Sql.get_graph_execution_stateincloud_pipelines_backend/api_server_sql.pyhas 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()callLine 758 (at
c0cc194):Row.tuple()has been deprecated since SQLAlchemy 2.0.19 in favour ofRow._tuple(). The project pinssqlalchemy>=2.0.51, where the old name still works but emits anSADeprecationWarningon every call. It's slated for removal in SQLAlchemy 2.1, at which point/api/executions/{id}/stateand/api/executions/{id}/graph_execution_statewould 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:
This fails
TestGetGraphExecutionState::test_direct_container_childrenandTestGetGraphExecutionState::test_three_level_mixed_statswith:2. Commented-out alternative query
Lines 675 to 703 are a fully commented-out version of
child_descendants_querythat joinsContainerExecution, with a note explaining it can't be used becauseContainerExecutionrows 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
row.tuple()withrow._tuple()and drop the first TODO. Keep the second TODO about SQLAlchemy 2.1, since it's still accurate.tests/test_execution_nodes_api_service.py::TestGetGraphExecutionStatecover the empty, direct-children, and three-level cases, and the full suite passes with-W error::DeprecationWarningafter 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.