Skip to content

Require marshmallow 4 or later - #107

Open
Marenz wants to merge 2 commits into
frequenz-floss:v1.x.xfrom
Marenz:marshmallow-4-compat
Open

Require marshmallow 4 or later#107
Marenz wants to merge 2 commits into
frequenz-floss:v1.x.xfrom
Marenz:marshmallow-4-compat

Conversation

@Marenz

@Marenz Marenz commented Dec 23, 2025

Copy link
Copy Markdown
Contributor

The code has required marshmallow 4 since 4ddc29d; the metadata still advertises 3.x, which cannot import.

Correct the dependency floor and document the required consumer upgrade.

@Marenz
Marenz requested a review from a team as a code owner December 23, 2025 09:16
@github-actions github-actions Bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:code Affects the code in general labels Dec 23, 2025
@Marenz
Marenz force-pushed the marshmallow-4-compat branch from bb481ba to 8245824 Compare December 23, 2025 09:20

Copilot AI left a comment

Copy link
Copy Markdown

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 adds backwards-compatible support for marshmallow 4.x by migrating from schema context-based serialization configuration to Python's ContextVar. The changes maintain full API compatibility while deprecating the old constructor-based approach.

Key changes:

  • Introduces serialize_as_string_default as a ContextVar for controlling serialization format globally
  • Deprecates the serialize_as_string_default constructor parameter with a DeprecationWarning
  • Adds backwards compatibility handling in _QuantityField.__init__ to check metadata for per-field configuration

Reviewed changes

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

File Description
src/frequenz/quantities/experimental/marshmallow.py Adds QuantitySchema.__init__ with deprecation logic and backwards compatibility for metadata-based field configuration
tests/experimental/test_marshmallow.py Adds two new tests to verify the deprecated constructor API works correctly with both True and False values
RELEASE_NOTES.md Documents the API changes, migration path, and rationale for the marshmallow 4.x compatibility update

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

Comment thread tests/experimental/test_marshmallow.py Outdated
Comment thread tests/experimental/test_marshmallow.py
Comment thread src/frequenz/quantities/experimental/marshmallow.py Outdated
Comment thread src/frequenz/quantities/experimental/marshmallow.py Outdated
@Marenz
Marenz force-pushed the marshmallow-4-compat branch from 8245824 to 9c30b12 Compare December 23, 2025 09:21
@github-actions github-actions Bot added the part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) label Dec 23, 2025
@Marenz
Marenz force-pushed the marshmallow-4-compat branch 5 times, most recently from 6b536ab to 8cca58a Compare December 23, 2025 10:32
@llucax llucax added the scope:breaking-change Breaking change, users will need to update their code label Jan 5, 2026
@llucax llucax added this to the v2.0.0 milestone Jan 5, 2026

@llucax llucax 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.

OK, this is a tricky one, and a reason why we might want to split marshmallow support into a separate package at some point.

For people using marshmallow, this is in fact a breaking change, as marshmallow 4.0 is required, not just optional, but for users not using marshmallow is not a breaking change.

Normally, if any part of the code is backwards incompatible, even if unused, should be a breaking release, I think this should be a breaking release too. The only reason we could try to sell this as a non-breaking release would be because it is an experimental package. But still, we are supposed to be committed to not break even those.

What we could do to keep this backwards compatible, is getting the installed marshmallow version and import a different module that is compatible with one or the other version conditionally based on that.

But I think in this case it makes sense to just bite the bullet and release it as a breaking change. For users not using marshmallow, it will be just changing the dependency and changing no code. For users using marshmallow, they are going through a marshmallow upgrade anyway, so fixing one more piece of code should not be too bad.

I'm not sure if keeping the deprecated ctor makes sense if we ship this in a breaking release anyway. It might ease the upgrading a bit for users if they don't need to make any other marshmallow-related updates, so we could keep it if we want to be extra nice, and only remove the deprecated ctor in v3, but I'm not sure it is worth the trouble.

@Marenz

Marenz commented Jan 12, 2026

Copy link
Copy Markdown
Contributor Author

I think I would be in favor of just breaking it then without a compatible c'tor :)

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
@Marenz
Marenz force-pushed the marshmallow-4-compat branch from 8cca58a to 081d31b Compare August 28, 2026 09:33
@Marenz Marenz changed the title Add backwards-compatible marshmallow 4.x support Require marshmallow 4 or later Aug 28, 2026
Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
@Marenz
Marenz requested review from llucax and a lite review from Copilot August 28, 2026 10:12
@Marenz

Marenz commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Resolved the three stale Copilot threads: they all concerned the serialize_as_string_default shim, which this PR no longer contains — the diff is now just the dependency floor correction plus the release note.

@Marenz
Marenz enabled auto-merge August 28, 2026 10:16

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread RELEASE_NOTES.md
## Bug Fixes

<!-- Here goes notable bug fixes that are worth a special mention or explanation -->
- The `marshmallow` optional dependency now requires version 4 or later. Consumers pinned to marshmallow 3.x must upgrade.
Comment thread pyproject.toml
Comment on lines 88 to 91
marshmallow = [
"marshmallow >= 3.0.0, < 5",
"marshmallow >= 4, < 5",
"marshmallow-dataclass >= 8.0.0, < 9",
]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:code Affects the code in general part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) scope:breaking-change Breaking change, users will need to update their code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants