Merge extensions into core package as PEP 771 extras#1054
Conversation
Signed-off-by: Sergio Herrera <627709+seherv@users.noreply.github.com>
Signed-off-by: Sergio Herrera <627709+seherv@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1054 +/- ##
==========================================
- Coverage 86.63% 81.98% -4.66%
==========================================
Files 84 116 +32
Lines 4473 9462 +4989
==========================================
+ Hits 3875 7757 +3882
- Misses 598 1705 +1107 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 121 out of 201 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
pyproject.toml:1
- The dependency was renamed from
msgpack-python(legacy, last release 0.5.6, capped at<1.0.0in the deletedext/*/pyproject.toml) tomsgpackwith>=1.0,<2.0. These are distinct PyPI projects with different APIs —msgpack1.x removedencoding=/use_bin_typedefaults and changedUnpacker/packbkeyword arguments. Confirm thatdapr/ext/langgraph/dapr_checkpointer.pyanddapr/ext/strands/dapr_session_manager.pyactually use themsgpack(1.x) API and not themsgpack-python(0.x) API; otherwise serialization of existing checkpoints/sessions in production state stores will break silently on upgrade.
flask_dapr/init.py:1 - Importing
actorandappas names fromdapr.ext.flaskonly works if those submodules have already been bound as attributes of the parent package.dapr/ext/flask/__init__.pydoesfrom .actor import DaprActor/from .app import DaprApp, which sets the attributes as a side effect — but only when those imports succeed. If the optionalflaskdependency is missing,dapr.ext.flask.__init__raises a newImportErrorbefore bindingactor/app, and this line will then raiseImportError: cannot import name 'actor' from 'dapr.ext.flask', masking the intendedFutureWarning-then-helpful-error path. Consider importing the submodules explicitly (from dapr.ext.flask import actor, appon its own line, orimport dapr.ext.flask.actor as actor) and/or wrapping in a try/except that surfaces the original cause.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 121 out of 201 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
pyproject.toml:1
all = ["dapr[fastapi,...]"]is a self-referential dependency (the project depending on itself with extras). Some resolvers/installers may reject this or behave unexpectedly (circular/recursive requirement). Prefer makingallthe explicit union of third-party deps (duplicating the lists), or use a build-tool-supported mechanism to compose extras without introducing aRequires-Dist: dapr[...]entry.
Signed-off-by: Sergio Herrera <627709+seherv@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 122 out of 202 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pyproject.toml:1
allis declared as an extra that depends on the same project (dapr[...]). Self-referential extras can confuse resolvers and may lead to circular/duplicate requirements (especially when users runpip install "dapr[all]"). Prefer expandingallto the union of the third-party dependencies across the other extras (duplicate the dependency strings), soallis just a normal list of external requirements.
Signed-off-by: Sergio Herrera <627709+seherv@users.noreply.github.com>
4ac3c15 to
88cdef6
Compare
Signed-off-by: Sergio Herrera <627709+seherv@users.noreply.github.com>
# Conflicts: # dapr/ext/strands/AGENTS.md
Signed-off-by: Sergio Herrera <627709+seherv@users.noreply.github.com>
Signed-off-by: Sergio Herrera <627709+seherv@users.noreply.github.com>
sicoyle
left a comment
There was a problem hiding this comment.
few comments/questions. Overall, I am on a call with Cassie right now, bc we're supposed to release 1.18 today... so all of your 1.XX refs need to bump by one pls bc this will not make the 1.18 release.
| # Register submodule aliases so `from flask_dapr.actor import DaprActor` and | ||
| # `import flask_dapr.app` resolve without on-disk files. | ||
| sys.modules['flask_dapr.actor'] = actor |
There was a problem hiding this comment.
do you do this on all of these pkgs we're changing?
There was a problem hiding this comment.
Nope, just flask_dapr which is the only oddly named extension we have.
It was imported with import flask_dapr but all other extensions were imported as import dapr.ext.fastapi and I made this one consistent with that. This is a compatibility shim that actually installs dapr[flask] on a pip install flask_dapr==1.19 and it will only exist for one version. The forced re-export there is to keep it working until we finally deprecate it
| # Bundling the extensions into the core wheel brings their source under mypy's | ||
| # namespace walk for the first time. These modules carry pre-existing type | ||
| # errors that were previously hidden by the separate-workspace-package layout. | ||
| # Ignored here to keep this packaging PR scope-limited; clean up in a follow-up. |
There was a problem hiding this comment.
this just means linter/type issues that need fixing? can you create an issue for this pls and link
| limitations under the License. | ||
| """ | ||
|
|
||
| # TODO: remove in 1.20 (pre-1.19 dapr-ext-* / flask-dapr migration only). |
There was a problem hiding this comment.
can you actually update this to be 1.21. I just got on a call with cassie that we are cutting 1.18 today so 1.18 will not get these changes. This will be in for 1.19 so all of your 1.20 refs need to be 1.21 pls and any other 1.19 refs become 1.20 pls
There was a problem hiding this comment.
Hmm, the way I see this:
1.18: released already, onlydapr-ext-fastapi1.19.dev: this branch merged into main, hasdapr-ext-fastapianddapr[fastapi]coexisting1.19: same,dapr-ext-fastapianddapr[fastapi]. Users have this version's lifetime to start switching1.20: onlydapr[fastapi], the old ext package not released at all
So I wouldn't bump any versions in the files, unless I am off by one, but I started from the fact that dapr-ext-fastapi==1.18.0 exists today and that one version is enough time for users to update
There was a problem hiding this comment.
Agree with Sam, but I even think we need to bump it to 1.22. We support N-2 versions, so by the time 1.18 become unsupported we will be releasing 1.21. Up to that point, users might still be in 1.18, and only then, they will be 'forced' to update to 1.21. So worst case scenario, users migrate from 1.18 to 1.21 because 1.18 is unsupported. So I think we should assist this migration by keeping the legacy check until 1.21, and remove it in 1.22.
Does this make sense?
Also I think we can stop publishing individual ext packages in 1.19. Users installing 1.19 from scratch don't need it, and users migrating to 1.19 will get the warning. In neither case we need the ext package shipped in 1.19, am I right?
Signed-off-by: Sergio Herrera <627709+seherv@users.noreply.github.com>
Description
Moves all the
dapr-ext*packages into the maindaprpackage, and exposes them as extras.pip install dapr-ext-pkgnameis nowpip install "dapr[pkgname]".IMPORTANT: this is a breaking change, unlike the
dapr*-devremoval. All the old extension packages must be uninstalled manually before re-installing dapr. More info indapr/__init__.py,README.mdandRELEASE.md.Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #1026
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: