Skip to content

Restore Pipeline.show_graph, fix graph rendering, add tests - #114

Merged
brian-arnold merged 7 commits into
devfrom
eywalker/plt-1180-restore-pipelineshow_graph-method-with-tests
Mar 27, 2026
Merged

Restore Pipeline.show_graph, fix graph rendering, add tests#114
brian-arnold merged 7 commits into
devfrom
eywalker/plt-1180-restore-pipelineshow_graph-method-with-tests

Conversation

@eywalker

@eywalker eywalker commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Remove redundant VizGraphNode class — actual pipeline nodes already carry .label and .node_type
  • Replace all kernel_type references in GraphRenderer with node_type so render_graph works directly with pipeline._node_graph
  • Add Pipeline.show_graph() convenience method
  • Add comprehensive test coverage for graph rendering (26 tests)
  • Centralize @pytest.mark.postgres skip logic in root conftest.py with --postgres CLI opt-in flag
  • Update CI postgres workflow to pass --postgres

Fixes PLT-1180

Test plan

  • All 26 new graph rendering tests pass
  • Full test suite passes (2847 passed, 49 skipped, 0 errors)
  • PostgreSQL tests properly skipped without --postgres flag
  • CI postgres workflow passes with --postgres flag

🤖 Generated with Claude Code

eywalker and others added 2 commits March 26, 2026 23:03
…add show_graph

Remove the redundant VizGraphNode class — actual pipeline nodes already
carry .label and .node_type. Rename all kernel_type references in
GraphRenderer to node_type so render_graph works directly with
pipeline._node_graph. Add Pipeline.show_graph() convenience method.
Update tutorial notebook.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… via --postgres

Add test_graph_rendering.py covering GraphRenderer, render_graph,
render_graph_dark_theme, StyleRuleSets, and Pipeline.show_graph.

Centralize @pytest.mark.postgres skip logic in root conftest.py with
a --postgres CLI flag (opt-in). Update CI postgres workflow to pass
the flag. Remove duplicated skip hooks from subdirectory conftests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 26, 2026 23:23
@eywalker
eywalker requested a review from brian-arnold March 26, 2026 23:24
@codecov

codecov Bot commented Mar 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/orcapod/pipeline/graph.py 96.29% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Avoids blocking PRs on untestable rendering paths (matplotlib display,
graphviz file output) while still catching meaningful coverage gaps.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR restores and modernizes pipeline graph visualization by removing an unnecessary visualization node wrapper, switching the renderer to use the pipeline’s real node metadata (node_type/label), adding a Pipeline.show_graph() convenience API, and expanding automated coverage around graph rendering. It also centralizes PostgreSQL test opt-in behavior behind a --postgres flag and updates CI accordingly.

Changes:

  • Add Pipeline.show_graph() and update GraphRenderer to style/label nodes based on GraphNode.node_type.
  • Add a new tests/test_pipeline/test_graph_rendering.py suite covering graph rendering utilities and the new pipeline API.
  • Centralize @pytest.mark.postgres opt-in skipping in tests/conftest.py and pass --postgres in the PostgreSQL CI workflow.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/orcapod/pipeline/graph.py Removes legacy viz wrapper, adds Pipeline.show_graph(), and refactors GraphRenderer to use node_type/label.
tests/test_pipeline/test_graph_rendering.py Adds broad unit test coverage for graph rendering utilities and Pipeline.show_graph().
tests/conftest.py Introduces --postgres opt-in flag and default skipping of @pytest.mark.postgres tests.
tests/test_databases/conftest.py Updates documentation to reflect centralized postgres skip behavior.
tests/test_core/sources/conftest.py Removes duplicated postgres skip logic (now handled in root conftest) while keeping the psycopg stub behavior.
.github/workflows/run-postgres-tests.yml Runs postgres workflow tests with --postgres enabled.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1205 to 1207
if node.node_type in rules:
default_attrs.update(rules[node.node_type])

Copilot AI Mar 26, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_get_node_attributes() now keys style rules strictly by node.node_type, but DARK_THEME_RULES still uses the key "pod" for function nodes. Since FunctionNode.node_type == "function", dark-theme styling will never apply to function nodes. Rename the dark-theme key to "function" (and optionally keep "pod" as a backwards-compatible alias) so function nodes get themed correctly.

Suggested change
if node.node_type in rules:
default_attrs.update(rules[node.node_type])
# Determine which style rule key to use.
# Primary key is the node's node_type; fall back to "pod" for
# backward compatibility with themes that still use that key
# for function nodes.
style_key = node.node_type
if (
style_key == "function"
and style_key not in rules
and "pod" in rules
):
style_key = "pod"
if style_key in rules:
default_attrs.update(rules[style_key])

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 727f221. Renamed "pod""function" in DARK_THEME_RULES. No backward-compat alias needed (pre-v0.1.0 greenfield).

Comment on lines 1422 to 1426
"shape": "box",
"fontcolor": pod_main_fcolor,
"style": "filled,rounded",
"type_font_color": kernel_type_fcolor,
"type_font_color": node_type_fcolor,
},

Copilot AI Mar 26, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In StyleRuleSets.create_custom_rules(), the returned dict uses type_font_color keys (e.g. in the function rule). GraphRenderer expects typefontcolor in node_attrs (and the built-in rule sets also use typefontcolor), so these overrides won't affect the HTML label and may emit an unrecognized DOT attribute. Use typefontcolor consistently in the custom rules output.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 727f221. create_custom_rules now uses typefontcolor consistently, matching the built-in rule sets.

Comment on lines 1224 to 1228
**style_overrides,
) -> str:
# Get final styles (defaults + overrides)
styles = self._merge_styles(**style_overrides)

import graphviz

Copilot AI Mar 26, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Within generate_dot(), the global fontsize logic later in the function checks styles.get("font_size") but then reads styles["fontsize"]. This means passing fontsize=... won't take effect, and passing font_size=... can raise KeyError. Use a single key consistently (e.g. fontsize) and access it via .get(...) to avoid runtime errors.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 727f221. generate_dot now checks styles.get("fontsize") consistently, matching render_graph.

eywalker and others added 4 commits March 26, 2026 23:29
Codecov expects bare numbers, not percentage strings.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add tests that run the non-raw_output render_graph paths including
graphviz DOT compilation, output_path file writing, and show=True
with mocked matplotlib. Brings patch coverage for rendering code
well above 80%.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The secrets context is not available in job-level `if` expressions,
causing workflow validation to fail. Move the event-type check to
the job level and gate individual steps on the env var instead.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…, fontsize

- Rename DARK_THEME_RULES "pod" key to "function" to match node_type
- Use "typefontcolor" consistently in style rules dicts (was
  "type_font_color" in create_custom_rules)
- Fix font_size/fontsize key mismatch in generate_dot

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@brian-arnold
brian-arnold merged commit b1edaaf into dev Mar 27, 2026
8 checks passed
@eywalker
eywalker deleted the eywalker/plt-1180-restore-pipelineshow_graph-method-with-tests branch May 18, 2026 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants