Skip to content

refactor: dataclasses cleanup — off-wire @dataclass removals, NamedTuple upgrades, frozen DispatchOptions#949

Merged
lchoquel merged 10 commits into
devfrom
refactor/Dataclasses
May 30, 2026
Merged

refactor: dataclasses cleanup — off-wire @dataclass removals, NamedTuple upgrades, frozen DispatchOptions#949
lchoquel merged 10 commits into
devfrom
refactor/Dataclasses

Conversation

@lchoquel
Copy link
Copy Markdown
Member

@lchoquel lchoquel commented May 29, 2026

Summary

  • Replaces stdlib @dataclass usage on off-wire data shapes with the lowest-churn correct form per site: plain NamedTuple upgrades for simple value tuples, and a frozen pydantic dataclass for DispatchOptions.
  • Removes off-wire stdlib @dataclass decorators (Phases 1-4) and converts simple internal contexts (e.g. ReportingManager._EventLogContext) to NamedTuple.
  • Migrates DispatchOptions to a frozen pydantic dataclass (Phases 5-6).
  • Merges in dev (pathlib.Path refactor refactor: use pathlib.Path for filesystem path handling #948); the single conflict in pipelex/reporting/reporting_manager.py was resolved to combine the runtime Path import + Path(...) call-site wrapping with the _EventLogContext NamedTuple conversion.

Test plan

  • make agent-check — ruff, Pyright (0 errors), Mypy (clean)
  • make tb — boot/config sanity
  • make agent-test — full unit suite passes

🤖 Generated with Claude Code


Summary by cubic

Cleans up off‑wire data shapes by removing stdlib @dataclass and making types explicit, and widens Temporal serialization so Pydantic dataclasses round‑trip with type preservation. Also pins kajson to 0.6.0 from PyPI.

  • New Features

    • Pydantic dataclasses (incl. optionals/lists) now serialize via BaseModelPayloadConverter through kajson with type preservation.
    • Added regression tests for direct converter calls and an end‑to‑end workflow/activity round‑trip.
  • Refactors

    • Replaced bare tuple returns with NamedTuple (StructureNameAndRefine, CrossPackageRef) and converted simple internal contexts to NamedTuple.
    • Migrated DispatchOptions to a frozen Pydantic dataclass; kept temporalio optional by binding RetryPolicy = Any at runtime.
    • Converted ErrorPagesReport and VariableReference to Pydantic dataclasses; CustomClassInfo and RegistrationFailure to frozen BaseModel.
    • Updated dependencies: pin kajson==0.6.0 from PyPI (dropped git source). Added wip/dataclass-refactor-story.html with context on form selection and the converter change.

Written for commit e724a17. Summary will update on new commits.

Review in cubic

lchoquel and others added 7 commits May 29, 2026 09:49
…le upgrades (Phases 1-4)

- Phase 2: bare-tuple returns -> NamedTuple (StructureNameAndRefine, CrossPackageRef)
- Phase 3: off-wire stdlib dataclasses -> per-site forms: _EventLogContext (NamedTuple),
  ErrorPagesReport + VariableReference (pydantic dataclass), CustomClassInfo (frozen BaseModel)
- Phase 4: RegistrationFailure -> frozen BaseModel (type-distinct from its tuple union arm)
- Only DispatchOptions (Phase 5) remains a stdlib dataclass
- Ledger: form-selection guide rewritten (pydantic dataclass first-class off-wire); perf phase dropped

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…old-start handoff

Phases 1-4 landed in 72e1800; record that and keep NEXT ACTION = Phase 5.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Phases 5-6)

Convert the last stdlib @DataClass in pipelex/ (DispatchOptions in
config_temporal.py) to a frozen pydantic dataclass, completing HARD RULE 1 of
the data-class refactor (zero stdlib @DataClass remain in pipelex/).

An `else: RetryPolicy = Any` runtime binding on the TYPE_CHECKING guard keeps
the module importable and constructible without the optional `temporal` extra
(the retry_policy field annotation resolves to Any at runtime); type checkers
still see the real temporalio RetryPolicy. Add a subprocess regression test
that blocks temporalio, then imports config_temporal and constructs a
DispatchOptions -- complementing the existing AST optional-dep scan, which
would not catch a revert to a bare forward ref.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…old-start handoff

Reflect that the Phase 5/6 work landed as 326d323 (committed + pushed), and
record the post-implementation code-review doc fixes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves one conflict in pipelex/reporting/reporting_manager.py: combines
dev's runtime pathlib.Path import + Path() call-site wrapping (#948) with this
branch's _EventLogContext NamedTuple conversion, dropping the now-unused
TYPE_CHECKING guard.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 29, 2026

Greptile Summary

This PR cleans up internal data holder types and broadens Temporal payload conversion. The main changes are:

  • Converts simple internal records to NamedTuple, frozen models, or pydantic dataclasses.
  • Adds pydantic dataclass routing in the Temporal payload converter.
  • Makes DispatchOptions frozen while keeping Temporal optional-dependency importability.
  • Adds converter and workflow round-trip coverage for pydantic dataclass payloads.

Confidence Score: 4/5

This is close, but the dependency wiring should be fixed before merging.

  • The code changes for the internal type conversions look behavior-preserving.

  • The new Temporal dataclass path depends on kajson behavior that is only available through a sibling editable checkout.

  • Normal installs can resolve a different kajson version than the one used by the new tests.

  • pyproject.toml

  • uv.lock

Important Files Changed

Filename Overview
pipelex/temporal/temporal_data_converter.py Adds pydantic dataclass detection and kajson-based Temporal serialization.
pipelex/temporal/config_temporal.py Moves DispatchOptions to a frozen pydantic dataclass with a runtime optional-dependency fallback.
pyproject.toml Adds an editable local kajson source while the runtime dependency still names kajson 0.5.0.
uv.lock Locks kajson to an editable sibling checkout instead of a registry artifact.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
pyproject.toml:58
**Avoid local dependency source** This PR adds Temporal support that depends on kajson 0.6.0 behavior, but the committed project metadata still declares `kajson==0.5.0` while overriding it with an editable `../kajson` checkout. A normal checkout of this repo without that sibling directory can fail to sync from the lockfile, and installs that follow the declared dependency can still get kajson 0.5.0, so the new dataclass payload path can run against the old serializer behavior. Please publish/bump the runtime dependency to the required kajson version and avoid committing the repo-local editable source as the way this feature is enabled.

Reviews (1): Last reviewed commit: "Merge branch 'dev' into refactor/Datacla..." | Re-trigger Greptile

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5c690f49ce

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread pyproject.toml Outdated
Comment thread pyproject.toml Outdated
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 16 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread pyproject.toml Outdated
lchoquel and others added 3 commits May 29, 2026 19:16
The [tool.uv.sources] git rev already resolves to kajson 0.6.0, but the
declared constraint still said ==0.5.0. Brings the metadata in line with
reality so the version mismatch can't bite a pip/PyPI consumer once 0.6.0
ships to PyPI.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…d refactor doc

kajson 0.6.0 is now published to PyPI, so remove the [tool.uv.sources] git
pin and resolve kajson==0.6.0 from the registry — closing the release gate
that the dataclass refactor depended on.

Also adds wip/dataclass-refactor-story.html, a co-developer walkthrough of
the data-class best-practices refactor: the per-site form-selection guide and
the Temporal payload converter widening (pydantic dataclasses now wire-legal,
BaseModel path byte-identical).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lchoquel lchoquel merged commit 659fd9b into dev May 30, 2026
26 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators May 30, 2026
@lchoquel lchoquel deleted the refactor/Dataclasses branch May 31, 2026 09:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant