Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Problem
-------
Importing anything from the deprecated `griptape.drivers` namespace
emitted one static message that always used `OpenAiChatPromptDriver` as
its worked example. A user importing, say,
`GriptapeCloudEventListenerDriver` was told to write
`from griptape.drivers.prompt.openai import OpenAiChatPromptDriver` —
migration guidance for a symbol they never touched.
Root cause
----------
`DeprecationModuleWrapper.__getattr__` warned with a fixed
`_deprecation_message` before resolving the attribute, so the message
could not depend on what was actually imported. The example lived in the
`deprecation_message` string in `griptape/drivers/__init__.py`.
Approach
--------
Resolve the attribute from the real module first, then work out which
package re-exports it and append
`Use `from <package> import <name>` instead.`
The package matters. `value.__module__` names the private implementation
module a symbol is *defined* in
(`griptape.drivers.prompt.openai_chat_prompt_driver`), not the
provider-specific package the message tells users to import from
(`griptape.drivers.prompt.openai`), which is the form
`griptape/drivers/__init__.py` itself uses and the form the docs prefer
215 times to 16. So instead of using `__module__` directly, look for the
already-imported subpackage of the defining module's parent that
re-exports the same object, preferring the most specific match, and fall
back to the generic message when no such package exists. Values with no
usable `__module__` (plain constants) also fall back rather than
inventing a target.
The misleading static OpenAI example was removed from the wrapper's base
message since the replacement is now derived per symbol.
Verification
------------
`pytest tests/unit/drivers/prompt/test_base_prompt_driver.py
tests/unit/utils/test_deprecate.py` reports 31 passed (27 on the base
commit). One new test walks all 114 distinct names in
`griptape.drivers.__all__`, asserts each one gets a replacement clause
naming itself, and then imports the suggested target and asserts it is a
package whose attribute is the identical object. Restoring only
`griptape/utils/deprecation.py` and `griptape/drivers/__init__.py` to
their pre-fix state while keeping the new tests fails 3 of them, so they
genuinely cover this change; the fourth
(`test_wrapper_warns_without_a_replacement_for_values_without_a_module`)
is a guard against a fabricated path and passes either way.
`ruff format --check` reports all four touched files already formatted;
`ruff check` reports only the pre-existing `CPY001` (missing copyright
notice) that the base commit already reports for the same four files.
Impact
------
Deprecation warnings become directly actionable and point at the public
package rather than at internal module layout. Anything asserting on the
exact old warning text will need updating; the `DeprecationWarning`
category, `stacklevel`, and the set of attributes that warn are
unchanged. Attribute resolution now happens before the warning is
emitted rather than after.
Downstream projects still on the legacy namespace will see more
warnings. Python's `default` warning filter de-duplicates on message
text, so several names imported in one statement used to collapse to a
single warning; a per-symbol message defeats that collapse and emits one
warning per name. Measured in fresh processes with
`warnings.simplefilter("default")`, a three-name `from griptape.drivers
import ...` goes from 1 warning to 3. No import site in this repo is
affected: the only `.py` matches are two single-name imports in
`tests/unit/drivers/prompt/test_base_prompt_driver.py`.
breken-ai
force-pushed
the
fix/deprecated-import-warning-target
branch
from
September 10, 2026 16:27
d1d72f1 to
676597b
Compare
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
1 participant
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.
Describe your changes
The symptom
Importing any name from the deprecated
griptape.driversnamespace emits one fixedDeprecationWarningwhose worked example is alwaysOpenAiChatPromptDriver, regardless of what you actually imported.Import
GriptapeCloudEventListenerDriverone825448bee212745aa58aa09f628caa5f8a0f9efand you are told:That is migration guidance for a symbol you never touched, in a package you do not want.
griptape.drivers.__all__exports 131 entries (114 distinct names — a handful repeat), so for 113 of the 114 the hint is about something else entirely. This fires at precisely the moment a user is trying to work out where their import moved to.The root cause
DeprecationModuleWrapper.__getattr__warns with a fixed string before resolving the attribute, so the message cannot depend on what was imported —griptape/utils/deprecation.py:24-31:The single static example lives in the
deprecation_messagestring atgriptape/drivers/__init__.py:272-277.What this change does
Resolves the attribute first, works out which package re-exports it, and appends
Use `from <package> import <name>` instead.The now-redundant static OpenAI example is removed from the base message.After:
Full diff: 4 files changed, 105 insertions(+), 5 deletions(-) —
griptape/utils/deprecation.py+36/-2,griptape/drivers/__init__.py+1/-2 (removing the static example), and the remaining +68/-1 in the two test files.Why not just use
value.__module__Because that names the private implementation module a symbol is defined in (
griptape.drivers.prompt.openai_chat_prompt_driver), not the "provider-specific package" the message asks users to import from (griptape.drivers.prompt.openai). Using it would have the warning contradict its own preamble and steer users at internal layout you are free to reshuffle.The repo consistently prefers the package form. Measured, not assumed:
and
griptape/drivers/__init__.pyitself has 111 relative imports, of which 110 go through packages and exactly 1 through a flat module (from .sql.sql_driver import SqlDriver, line 55). (The linked issue quotes that as "111 times against 1 flat module" — 111 is the total, not the package-form count. The direction of the argument is unchanged; I would rather correct my own number here than let it stand.)griptape.drivers.prompt.openaiis also exactly the form the deleted example used, so nothing regresses for the OpenAI case — verified in a fresh process on this branch:So
_replacement_packagelooks for the already-imported subpackage of the defining module's parent that re-exports the same object, prefers the most specific match, and falls back to the generic message when none exists.The trade-offs a reviewer should weigh
These are the reasons you might reject this as written, and I would rather list them than have you find them:
It is ~25 lines of runtime machinery in a warning path. An explicit name-to-package mapping would be simpler to read but needs hand-maintenance. Dropping the replacement clause entirely and just naming the symbol is a third option. The linked issue asks you to choose; I implemented one so there is something concrete to look at.
It depends on the package already being in
sys.modules. It never imports anything itself. Forgriptape.driversthis is safe becausegriptape/drivers/__init__.pyimports every provider package at module load, before the wrapper is installed — hence the fresh-process results above. For any other future user ofDeprecationModuleWrapperthat does not eagerly import, the fallback would produce the plain generic message.DeprecationModuleWrappercurrently has exactly one non-test user in the repo (griptape/drivers/__init__.py), so nothing else is affected today.Attribute resolution now happens before the warning is emitted rather than after. Consequence:
from griptape.drivers import NotARealNamenow raisesAttributeErrorwithout first emitting a spurious deprecation warning. I consider that an improvement, but it is a behaviour change and you may not.Anything asserting on the exact old warning text will need updating. The
DeprecationWarningcategory, thestacklevel, and the set of attributes that warn are all unchanged.A multi-name import now emits one warning per name instead of one per statement. Python's
defaultwarning filter de-duplicates on (message text, category, module, lineno). At base every deprecated name emits the same string, so several names imported in one statement collapse to a single warning. A per-symbol message has different text for each name, so nothing collapses. Measured in fresh processes withwarnings.simplefilter("default")— which is whatpython -W defaultand pytest'sDeprecationWarningdisplay give you:So per import site the count goes from 1 to the number of distinct names imported. That is the direct consequence of naming each symbol — there is no way to give per-symbol guidance and still collapse to one message — and it applies equally to the "just name the symbol" alternative in item 1, since that also varies the text per name. But it is a real increase in deprecation noise for anyone still on the legacy namespace: a downstream project with
from griptape.drivers import A, B, C, D, Ein its code goes from 1 to 5 lines in every CI log that surfacesDeprecationWarning. The cost lands on downstream users, not here —git grep "from griptape.drivers import" -- '*.py'matches onlytests/unit/drivers/prompt/test_base_prompt_driver.py, twice, both single-name; every other match in the repo (12) is a prose example inMIGRATION.md. If you consider the extra volume worse than the wrong worked example, that is a reason to prefer leaving the message alone.Issue ticket number and link
Closes #2334 (#2334)
Testing
Python 3.12.12, ruff 0.16.6, run from a clean checkout of this branch on top of
e825448.Targeted unit tests — 27 passed on the base commit, 33 on this branch:
Revert proof. Restoring only
griptape/utils/deprecation.pyandgriptape/drivers/__init__.pyto their state ate825448while keeping the new tests:Restoring the fix returns it to
33 passed in 0.18s.Branch coverage. Codecov flagged the
parent is None or parent is self or parent is self._real_moduleguard in_replacement_packageas uncovered, so two tests now exercise it: one where the defining module's parent package is not insys.modules, and one where that parent is the wrapped module itself. Both return the bare deprecation message with noUse ... instead.suffix. They do not fail againste825448(base has no replacement logic at all, so the bare message is already correct there); they were verified by deleting the two guard lines on this branch, which fails both:griptape/utils/deprecation.pyis now at 100% statement and branch coverage (coverage report --show-missing:31 stmts, 0 miss, 6 branch, 0 partial).Honesty note on the fourth new test. This branch adds 4 test nodes. Only 3 are regression tests — the ones listed above. The fourth,
test_wrapper_warns_without_a_replacement_for_values_without_a_module, passes against the unpatched source too. It is a guard against the code inventing a replacement path for a value with no usable__module__(a plain constant), not evidence that this change works. I am listing it separately rather than counting it toward the proof.Exhaustive check of every deprecated name. The strongest test here walks all 114 distinct names in
griptape.drivers.__all__, extracts the suggested package from each warning, imports it, and asserts it is a package whose attributeisthe identical object. I also re-ran that walk as a standalone script outside the test suite:Zero names fall back to the generic message, and none is pointed at a flat implementation module.
Before/after on the reported symptom, same script as in the linked issue:
The warning appears twice on both refs, because
from X import Yon a module wrapper performs two attribute lookups. That is pre-existing onmainand unchanged by this PR; I am pasting the real doubled output rather than a tidied single line.Formatting:
Lint — 4 findings, all
CPY001, and the base commit reports the same 4 for the same 4 files. No new findings:Whitespace:
git diff --check e825448is clean (exit 0).Full unit suite, to show nothing else moved.
--continue-on-collection-errorsis needed in my environment because 29 test modules fail to import for lack of optional extras (boto3,pypdf, and similar), which would otherwise abort the run before a single test executed:(Wall-clock times are omitted from those two lines because they swing with machine load and prove nothing.)
Those 82 failures and 248 errors are pre-existing on
e825448in my environment (missing optional extras), not caused by this branch. I sorted theFAILED/ERRORnode-id lines from both runs — 330 lines each — and diffed them: identical. The only delta is +4 passing nodes, which are the new test cases. This matters more than usual here, because a mistake in__getattr__ongriptape.driverscould plausibly break imports across the whole suite.What was not verified
Stated plainly, because understating is safer than overstating:
Import-order sensitivity was reasoned about, not fuzzed.
_replacement_packagereadssys.modulesandvars(parent), so in principle its answer depends on what has been imported. I confirmed the four spot-checks above in fresh processes, and the 114-name walk resolves cleanly, but I did not test under a randomized import order.pytest-randomlyis not installed in this environment, so test ordering here is deterministic and would not have caught such a dependency anyway.Only
griptape.driverswas exercised. That is the only non-test user ofDeprecationModuleWrapperin the repo today, but the class is generic and I have not tested it against a namespace that does not eagerly import its subpackages.The 17 duplicate entries in
__all__were not investigated.__all__has 131 entries and 114 distinct names. The duplicates are pre-existing onmainand untouched by this PR; the test deduplicates before walking.make checkwas run, but with drifting tool versions. All four legs —check/format,check/lint,check/types(pyright),check/spell(typos) — were run repo-wide one825448and on this branch. Each of the four outputs is byte-identical between the two refs, so the new_replacement_packagesignature (-> str | None) adds no pyright diagnostic:Those absolute counts are large because my local tools are newer than the versions
uv.lockpins (ruff 0.16.6 vs 0.15.15, pyright 1.1.413 vs 1.1.410, typos 1.50.1 vs 1.47.0), so they are not what CI will report — the pre-existing findings are an artifact of that drift. The load-bearing part is the base-to-branch delta, which is zero, and none of the four files this PR touches is in the 55-file "would reformat" set.make check/formatalso runsmdformat --check .github/ docs/;mdformatis not installed here, but this branch touches neither directory.One interpreter, one OS. Python 3.12.12 on macOS only. The
>=3.10, <4matrix and Windows were not exercised. The newstr | Nonereturn annotation is evaluated at runtime and needs 3.10+, which matches the declared floor atpyproject.toml:6, but I did not run a 3.10 interpreter to confirm.The full unit suite is not green to begin with in this environment, so "no regressions" here means "the failure set is byte-identical to base", not "everything passes".
No integration tests.
make test/integrationexecutes documentation code blocks and needs live provider credentials. Not run.