feat(graph): support multi-stamp execution graphs#250
Open
geoberle wants to merge 1 commit into
Open
Conversation
geoberle
force-pushed
the
stamp-validation
branch
from
ea75893 to
9a6cf99
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the pipelines graph tooling to represent and execute multi-stamp topologies by duplicating stamped services per stamp while deduplicating unstamped nodes. It also changes Service.Stamped to *bool to preserve explicit user intent (stamped: false) and introduces per-stamp keying in graph identifiers/metadata.
Changes:
- Changed
topology.Service.Stampedfromboolto*bool, updated stamping propagation, and added validation for explicit opt-outs under stamped parents. - Added stamp-aware graph identity (
Identifier.Stamp) plus composite keys for steps/resource groups, and introducedForStampedEntrypoint+mergeStampedto merge per-stamp graphs. - Added
ConfigResolver.GetRegionStampConfigurationto re-resolve configuration templates using a different stamp value.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pipelines/topology/types.go | Switches Stamped to pointer semantics and updates propagation/validation logic. |
| pipelines/topology/types_test.go | Adds/updates tests for pointer-based stamped propagation and validation cases. |
| pipelines/graph/graph.go | Introduces Stamp in identifiers and refactors graph lookups to use composite keys; adds ForStampedEntrypoint. |
| pipelines/graph/merge.go | Adds per-stamp graph merge implementation to combine per-stamp graphs into one execution graph. |
| pipelines/graph/merge_test.go | Adds tests for per-stamp merge behavior (node duplication, rewiring, and RG/step selection). |
| config/config.go | Adds API to resolve config for a region using an alternate stamp and stores resolver inputs for re-resolution. |
| config/config_test.go | Adds test coverage for stamp-specific config resolution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
geoberle
force-pushed
the
stamp-validation
branch
from
9a6cf99 to
5ad5152
Compare
geoberle
force-pushed
the
stamp-validation
branch
from
5ad5152 to
cb0be5e
Compare
geoberle
force-pushed
the
stamp-validation
branch
from
cb0be5e to
fc1c8c7
Compare
12 tasks
Until now, the graph tooling had no way to represent stamping: one graph could only hold one copy of each service. This change introduces a ForStampedEntrypoint func that builds a graph that holds duplicated stamped services. This graph can be executed by templatize in a full parallel manner. Additional: Making Stamped a *bool fixes a gap in user intent detection: with a plain bool, a child explicitly setting stamped: false was indistinguishable from "unset". PropagateStamped would silently override it to true, and the validator couldn't catch the contradiction. With pointer semantics, explicit false is preserved, and validation rejects it with a clear error — so invalid user intent doesn't go unnoticed.⚠️ This contains breaking changes on the Graph struct so it is able to represent stamped ServiceGroups. Don't merge this PR before a PR is ready in sdp-pipelines to adapt to the changes (available soon). What changed: - Stamped on Service is now *bool — explicit false preserved, validated - Identifier, ResourceGroupKey, and StepKey carry an optional Stamp field to disambiguate per-stamp nodes/metadata - Graph.Steps and Graph.ResourceGroups use composite keys instead of nested maps - mergeStamped merges per-stamp graphs: deduplicates unstamped nodes, adds stamped copies with correct parent/child rewiring - ConfigResolver.GetRegionStampConfiguration re-resolves config with a different stamp value - ForStampedEntrypoint ties it together — builds one graph per stamp, merges them
geoberle
force-pushed
the
stamp-validation
branch
from
fc1c8c7 to
5f52dbb
Compare
|
|
||
| // GetStep returns the step for a node, using the node's Stamp field to select per-stamp steps. | ||
| func (c *Graph) GetStep(node Identifier) (types.Step, bool) { | ||
| s, exists := c.Steps[StepKey{Stamp: node.Stamp, ServiceGroup: node.ServiceGroup, ResourceGroup: node.ResourceGroup, Step: node.Step}] |
Contributor
There was a problem hiding this comment.
Why do we want to distinguish between StepKey and Identifier? Aren't they the same thing?
| return cfg, nil | ||
| } | ||
|
|
||
| func (cr *configResolver) GetRegionStampConfiguration(region, stamp string) (types.Configuration, error) { |
Contributor
There was a problem hiding this comment.
Prefer to disallow setting the stamp during GetResolver() and defer it to the GetRegionConfiguration() call where it should be required?
| return ForEntrypoints(topo, []*topology.Entrypoint{entrypoint}, pipelines) | ||
| } | ||
|
|
||
| // ForStampedEntrypoint generates a stamp-aware execution graph. It builds a |
Contributor
There was a problem hiding this comment.
When would you ever not use this? Why keep the previous ones around?
|
|
||
| // GetResourceGroup returns the resource group metadata for a node, using the | ||
| // node's Stamp field to select per-stamp metadata. | ||
| func (c *Graph) GetResourceGroup(node Identifier) (*types.ResourceGroupMeta, bool) { |
Contributor
There was a problem hiding this comment.
I probably wouldn't do this - node Identifier is over-specified to select a resource group (fields are in there that the caller can pass but will be ignored). If you want to refactor the nested maps (I am not sure it is worth it, but feel free), you need strong types for these Get* calls
| // | ||
| // The stamps map keys are stamp identifiers (e.g. "1", "2", "3"). Each value | ||
| // is a graph produced by ForEntrypoint for that stamp's configuration. | ||
| func mergeStamped(stampGraphs map[string]*Graph) *Graph { |
Contributor
There was a problem hiding this comment.
Somehow I feel like we should be able to insert the correct nodes as necessary from the get-go, as opposed to having to run this de-dupe and merge process
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Until now, the graph tooling had no way to represent stamping: one graph could only hold one copy of each service.
This change introduces a ForStampedEntrypoint func that builds a graph that holds duplicated stamped services. This graph can be executed by templatize in a full parallel manner.
Additional: Making Stamped a *bool fixes a gap in user intent detection: with a plain bool, a child explicitly setting stamped: false was indistinguishable from "unset". PropagateStamped would silently override it to true, and the validator couldn't catch the contradiction. With pointer semantics, explicit false is preserved, and validation rejects it with a clear error — so invalid user intent doesn't go unnoticed.
It is necessary to merged these PRs to adapt the the change
What changed: