Repository navigation
models: deprecate the last undecorated dataset builder, and say what to use - #7
Merged
Merged
Conversation
…to use MinimalDataset was the only concrete builder without a @deprecated marker, so it was the one path that could still be adopted without warning. It has one now. The four existing messages said a builder "may be removed in a future release" without naming a replacement, which tells a caller to stop but not where to go. All of them now point at the same thing: build a Dataset and pass it to client.datasets.create(), with client.jobs.list() as the live parameter schema. That is the reason the builders are going in the first place, since each one duplicates a job's schema in Python and drifts when the job changes. BaseDatasetBuilder is deliberately left undecorated. @deprecated on an ABC fires at class-definition time rather than on use, so decorating it would emit a warning for every subclass as soon as md_python is imported, including for callers that never touch a builder. Its docstring carries the note instead, and a test pins the import-time silence so nobody adds the decorator later without noticing.
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.
What
MinimalDatasetwas the only concrete builder without a@deprecatedmarker, so it was the one path a caller could still adopt without any warning. It has one now.The four existing messages said a builder "may be removed in a future release" without naming a replacement, which tells a caller to stop but not where to go. All of them now point at the same thing:
That is the reason the builders are going in the first place. Each one duplicates a job's parameter schema in Python, so it drifts whenever the job changes server-side, while
client.jobs.list()is the live schema.Why
BaseDatasetBuilderis left undecorated@deprecatedon an ABC fires at class-definition time, not on use. Decorating the base would emit a warning for every subclass as soon asmd_pythonis imported, including for callers that never touch a builder. Verified:Its docstring carries the note instead, and
test_importing_md_python_does_not_warnpins the import-time silence so the decorator cannot be added later without a test failing.Tests
Two added. One asserts every concrete builder warns and that the message names
client.datasets.create(), so a future builder cannot be added with a dead-end message. The other pins import-time silence.317 passed.black,isortandmypyall clean across the repo, not just the changed files.Risk
Behavioural change is limited to a
DeprecationWarningonMinimalDataset, which is hidden by default in Python and visible under-Wor pytest. No signature, no return value and no wire format changes.