Skip to content

artifact: allow multiple parents for git-like merge lineage - #7972

Merged
EngHabu merged 3 commits into
mainfrom
haytham/artifact-multi-parent
Sep 3, 2026
Merged

artifact: allow multiple parents for git-like merge lineage#7972
EngHabu merged 3 commits into
mainfrom
haytham/artifact-multi-parent

Conversation

@EngHabu

@EngHabu EngHabu commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Why

Follow-up to #7963 on review feedback: a single parent_artifact models branching (a forest of chains) but not merges. Making the field repeated turns artifact version history into a full DAG, the git/Merkle-tree model — a version can combine several ancestors, not just derive from one.

What

  • ArtifactSpec.parent_artifactrepeated core.ArtifactVersionId parent_artifacts = 7 (same field number, max_items = 32).
  • ProducedArtifact.parent_artifactrepeated flyteidl2.core.ArtifactVersionId parent_artifacts = 6 (same field number, max_items = 32).
  • Docs: order is preserved and meaningful (first entry = primary parent, later entries are merged-in versions); duplicates after scope/name inheritance are rejected. Per-entry semantics unchanged from artifact: parent is a full artifact reference, not a bare version #7963: parents constrained to the child's own org/project/domain, empty key fields inherit the child's scope/name (bare {version} = same-name parent), stored as given, dangling legal, immutable.
  • Folds in artifact: bound ArtifactKey/ArtifactVersionId field lengths #7964 (now closed as superseded — it edited the same doc block): buf.validate max_len bounds on core.ArtifactKey (project/domain 64, name 255, org 63, matching ArtifactName) and ArtifactVersionId.version (255) — the artifact: parent is a full artifact reference, not a bare version #7963 Copilot feedback.
  • ListArtifactsRequest filter doc: the parent_name+parent_version pair now matches artifacts having the given version among their parents.
  • Regenerated go/python/ts/rust (buf 1.58.0, matching CI).

Breaking change — deliberate

Replaces the singular field in place rather than deprecating it: parent_artifact shipped in v2.0.47 days ago and has no consumers yet (confirmed with the one in-flight consumer, which is coding against the new shape).

Service implementation lands in the corresponding unionai/cloud PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_015JKranYEJG44SPwfPFqWnk

Copilot AI lite review requested due to automatic review settings September 3, 2026 17:36
@github-actions github-actions Bot added the flyte2 label Sep 3, 2026

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.

🔵 Needs a closer look

It introduces a deliberate protobuf API shape change across multiple generated language bindings and should receive final human verification for downstream compatibility and release impact.

Pull request overview

This PR updates the artifact lineage model to support git-like merge history by changing the singular parent_artifact reference into a repeated parent_artifacts list, allowing a full DAG of artifact version ancestry.

Changes:

  • Update protobuf schemas to make ArtifactSpec.parent_artifacts (field 7) and ProducedArtifact.parent_artifacts (field 6) repeated fields (preserving field numbers).
  • Clarify documentation to describe ordering semantics (primary parent first) and updated ListArtifacts filter semantics (“among their parents”).
  • Regenerate Go/Python/TypeScript bindings (and gateway swagger) to reflect the new repeated field shape.
File summaries
File Description
flyteidl2/artifact/artifact.proto Changes ArtifactSpec.parent_artifact to repeated parent_artifacts with updated DAG/ordering docs.
flyteidl2/task/common.proto Changes ProducedArtifact.parent_artifact to repeated parent_artifacts with updated merge/DAG docs.
flyteidl2/artifact/artifact_service.proto Updates ListArtifacts filter docs to reflect “among their parents” semantics.
gen/go/flyteidl2/artifact/artifact.pb.go Regenerated Go bindings for repeated ParentArtifacts.
gen/go/flyteidl2/artifact/artifact.pb.validate.go Regenerated Go validation to iterate over repeated parents.
gen/go/flyteidl2/task/common.pb.go Regenerated Go bindings for repeated ParentArtifacts.
gen/go/flyteidl2/task/common.pb.validate.go Regenerated Go validation to iterate over repeated parents.
gen/go/flyteidl2/artifact/artifact_service.pb.go Regenerated Go service bindings/comments for updated filter semantics.
gen/go/gateway/flyteidl2/connector/service.swagger.json Regenerated swagger to represent parent_artifacts as an array.
gen/python/flyteidl2/artifact/artifact_pb2.py Regenerated Python protobuf module for repeated parent_artifacts.
gen/python/flyteidl2/artifact/artifact_pb2.pyi Regenerated Python typing stubs for repeated parent_artifacts.
gen/python/flyteidl2/task/common_pb2.py Regenerated Python protobuf module for repeated parent_artifacts.
gen/python/flyteidl2/task/common_pb2.pyi Regenerated Python typing stubs for repeated parent_artifacts.
gen/ts/flyteidl2/artifact/artifact_pb.ts Regenerated TS bindings for repeated parentArtifacts.
gen/ts/flyteidl2/task/common_pb.ts Regenerated TS bindings for repeated parentArtifacts.
gen/ts/flyteidl2/artifact/artifact_service_pb.ts Regenerated TS service bindings/comments for updated filter semantics.
Review details

Files not reviewed (7)

  • gen/go/flyteidl2/artifact/artifact.pb.go: Generated file
  • gen/go/flyteidl2/artifact/artifact.pb.validate.go: Generated file
  • gen/go/flyteidl2/artifact/artifact_service.pb.go: Generated file
  • gen/go/flyteidl2/task/common.pb.go: Generated file
  • gen/go/flyteidl2/task/common.pb.validate.go: Generated file
  • gen/python/flyteidl2/artifact/artifact_pb2.py: Generated file
  • gen/python/flyteidl2/task/common_pb2.py: Generated file
  • Files reviewed: 9/18 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines 80 to 82
// declare the given artifact version among their parents — the downward
// direction of lineage, which the stored pointers alone cannot be followed
// in.

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.

Reworded in 95a181c: the sentence now reads "This serves the downward direction of lineage: the stored parent pointers only walk upwards, so finding a version's children requires this filter."

ArtifactSpec.parent_artifact and ProducedArtifact.parent_artifact become
repeated parent_artifacts (same field numbers, max 32 entries). One entry
is plain derivation; several record a merge, making version history a
full DAG (git/Merkle style) instead of a forest of chains. Order is
preserved and the first entry is the primary parent; duplicates (after
scope/name inheritance) are rejected. Per-entry semantics are unchanged:
same org/project/domain only, empty key fields inherit the child's scope
and name, stored as given, dangling parents legal, immutable.

Also folds in the #7963 review feedback (previously #7964): buf.validate
max_len bounds on core.ArtifactKey fields (project/domain 64, name 255,
org 63, matching ArtifactName) and ArtifactVersionId.version (255).

Breaking IDL change to a field nothing consumes yet: the singular field
shipped in v2.0.47 days ago and has no users, so it is replaced in place
rather than deprecated.

The parent_name+parent_version children filter on ListArtifacts is
unchanged; it now matches artifacts having the given version among their
parents.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015JKranYEJG44SPwfPFqWnk
Signed-off-by: Haytham Abuelfutuh <haytham@union.ai>
Copilot AI review requested due to automatic review settings September 3, 2026 17:41
@EngHabu
EngHabu force-pushed the haytham/artifact-multi-parent branch from dd928da to 9d262f9 Compare September 3, 2026 17:41

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.

🟡 Changes recommended

Several regenerated docs/comments still reference parent_artifact (singular) even though the API surface is now parent_artifacts, leaving documentation artifacts out of sync.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (9)

  • gen/go/flyteidl2/artifact/artifact.pb.go: Generated file
  • gen/go/flyteidl2/artifact/artifact.pb.validate.go: Generated file
  • gen/go/flyteidl2/artifact/artifact_service.pb.go: Generated file
  • gen/go/flyteidl2/core/artifact_id.pb.go: Generated file
  • gen/go/flyteidl2/task/common.pb.go: Generated file
  • gen/go/flyteidl2/task/common.pb.validate.go: Generated file
  • gen/python/flyteidl2/artifact/artifact_pb2.py: Generated file
  • gen/python/flyteidl2/core/artifact_id_pb2.py: Generated file
  • gen/python/flyteidl2/task/common_pb2.py: Generated file
  • Files reviewed: 11/23 changed files
  • Comments generated: 4
  • Review effort level: Lite

Comment thread flyteidl2/core/artifact_id.proto Outdated
Comment thread gen/go/flyteidl2/core/artifact_id.pb.go Outdated
Comment thread gen/go/gateway/flyteidl2/connector/service.swagger.json Outdated
Comment thread gen/ts/flyteidl2/core/artifact_id_pb.ts Outdated
wild-endeavor
wild-endeavor previously approved these changes Sep 3, 2026

@wild-endeavor wild-endeavor 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.

32 parents!

Copilot review feedback: the ArtifactKey doc still pointed at the
renamed field, and the ListArtifacts filter doc sentence read awkwardly.
Regenerated bindings.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015JKranYEJG44SPwfPFqWnk
Signed-off-by: Haytham Abuelfutuh <haytham@union.ai>

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.

🟡 Changes recommended

The new max_items = 32 contract for parent_artifacts is not enforced in the regenerated Go *.pb.validate.go validators (and the ArtifactKey doc comment needs a small correctness/clarity fix).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (9)

  • gen/go/flyteidl2/artifact/artifact.pb.go: Generated file
  • gen/go/flyteidl2/artifact/artifact.pb.validate.go: Generated file
  • gen/go/flyteidl2/artifact/artifact_service.pb.go: Generated file
  • gen/go/flyteidl2/core/artifact_id.pb.go: Generated file
  • gen/go/flyteidl2/task/common.pb.go: Generated file
  • gen/go/flyteidl2/task/common.pb.validate.go: Generated file
  • gen/python/flyteidl2/artifact/artifact_pb2.py: Generated file
  • gen/python/flyteidl2/core/artifact_id_pb2.py: Generated file
  • gen/python/flyteidl2/task/common_pb2.py: Generated file
  • Files reviewed: 11/23 changed files
  • Comments generated: 3
  • Review effort level: Lite

Comment on lines +705 to +708
for idx, item := range m.GetParentArtifacts() {
_, _ = idx, item

if all {

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.

This is the same PGV-vs-protovalidate split as the earlier constraint rounds: the .pb.validate.go files are legacy protoc-gen-validate output and enforce only PGV (validate.rules) annotations — they don't enforce ANY buf.validate rule, including the pre-existing max_len/min_len bounds throughout this repo (grep them: no length checks either). buf.validate constraints are enforced at runtime by protovalidate (see buf.gen.go.yaml's managed-mode dependency on buf.build/bufbuild/protovalidate). The serving implementation additionally enforces the 32-parent cap explicitly at the application layer.

Comment on lines +1386 to +1389
for idx, item := range m.GetParentArtifacts() {
_, _ = idx, item

if all {

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.

Same as the ArtifactSpec thread: .pb.validate.go is legacy PGV output that enforces no buf.validate rules at all (note it contains no enforcement for this message's pre-existing min_len bounds either); enforcement comes from protovalidate at runtime, plus an explicit application-layer cap in the serving implementation.

Comment thread flyteidl2/core/artifact_id.proto Outdated
Comment on lines +11 to +14
// Project and domain and suffix needs to be unique across a given artifact store.
string project = 1;
string domain = 2;
string name = 3;
string org = 4;
// Length bounds match artifact.ArtifactName; fields stay optional because an
// empty field means "inherit from context" in several uses (e.g.
// artifact.ArtifactSpec.parent_artifacts).

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.

Reworded in the latest commit: the doc now reads "Identifies one artifact within an artifact store: the (org, project, domain, name) tuple must be unique." (bindings regenerated).

Copilot review feedback: name the actual identity tuple instead of the
old 'project and domain and suffix needs to be unique' sentence.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015JKranYEJG44SPwfPFqWnk
Signed-off-by: Haytham Abuelfutuh <haytham@union.ai>
Copilot AI review requested due to automatic review settings September 3, 2026 18:48

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.

🔵 Needs a closer look

It introduces a deliberate breaking IDL change with broad regenerated output across multiple languages, warranting final human review of compatibility and release impact.

Review details

Files not reviewed (9)

  • gen/go/flyteidl2/artifact/artifact.pb.go: Generated file
  • gen/go/flyteidl2/artifact/artifact.pb.validate.go: Generated file
  • gen/go/flyteidl2/artifact/artifact_service.pb.go: Generated file
  • gen/go/flyteidl2/core/artifact_id.pb.go: Generated file
  • gen/go/flyteidl2/task/common.pb.go: Generated file
  • gen/go/flyteidl2/task/common.pb.validate.go: Generated file
  • gen/python/flyteidl2/artifact/artifact_pb2.py: Generated file
  • gen/python/flyteidl2/core/artifact_id_pb2.py: Generated file
  • gen/python/flyteidl2/task/common_pb2.py: Generated file
  • Files reviewed: 11/23 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment on lines +11 to +15
// Identifies one artifact within an artifact store: the (org, project,
// domain, name) tuple must be unique. Length bounds match
// artifact.ArtifactName; fields stay optional because an empty field means
// "inherit from context" in several uses (e.g.
// artifact.ArtifactSpec.parent_artifacts).
Comment on lines +153 to +162
// Optional artifact versions the produced version was derived from, copied
// verbatim into ArtifactSpec.parent_artifacts. Set by the producing task
// when it derives a new version from versions it consumed; one entry is
// plain derivation, several record a merge, so the resulting lineage forms
// a DAG instead of reading as a linear chain. Order is preserved: the first
// entry is the primary parent. Empty key fields inherit the produced
// artifact's own scope and name, so a bare {version: "v1"} declares the
// previous version of this same artifact as a parent; a set name records
// cross-artifact derivation. Only the name may differ: non-empty scope
// fields (org/project/domain) must match the produced artifact's own scope.
@EngHabu
EngHabu merged commit aeee504 into main Sep 3, 2026
25 checks passed
@EngHabu
EngHabu deleted the haytham/artifact-multi-parent branch September 3, 2026 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants