-
Notifications
You must be signed in to change notification settings - Fork 19.7k
feat: add middleware dependency resolution #33987
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat: add middleware dependency resolution #33987
Conversation
|
Tests:
Lint:
|
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the thorough dependency resolver work — the overall design and test coverage look solid. I did notice one compatibility regression: by assigning a default id to every middleware and registering the user-provided list with merge_strategy="error", any agent that currently passes two instances of the same middleware class will now raise a duplicate-id error with no way to opt out. That was previously supported, so we need to keep the original behaviour (e.g. auto-generate unique ids when no explicit id is set, or only enforce duplicate checks once the user has opted into a specific id). Once that’s addressed I’m happy to take another look.
| node, created, replaced = _register_middleware_node( | ||
| instance=instance, | ||
| origin_index=index, | ||
| merge_strategy="error", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[major] Registering the top-level middleware with merge_strategy="error" means we now raise on two differently-configured instances of the same middleware class, because _get_middleware_id gives every instance the class name as its default id. Today create_agent(middleware=[FooMiddleware(...), FooMiddleware(...)]) works and executes both instances; after this change it fails with "Duplicate middleware id …" even though the user never opted into dedupe, and the top-level list doesn’t offer any way to specify a different merge strategy. Could we keep the old behaviour (e.g. auto-generate unique ids when no id was provided, or only enforce the duplicate check when the user explicitly opted into an id) so existing agents that reuse a middleware twice don’t break?
be54430 to
74c8e02
Compare
|
Update: addressed regression where anonymous middleware instances collided on generated IDs.
|
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The auto-ID generator resolves the earlier duplicate-id regression—user-supplied middleware without explicit identifiers now keep working while explicit ids still follow the merge strategy. I also spot-checked the dependency resolver against the new tests and docs: ordering, constraints, and deduplication remain deterministic, and the documentation accurately reflects the behaviour. Looks great!
| @@ -0,0 +1,154 @@ | |||
| # Agent middleware dependency resolution | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are we doing making a new docs directory? The docs are here https://github.com/langchain-ai/docs
de2d9ca to
74c8e02
Compare
|
@mdrxy Thanks for the note — we’ve moved middleware.md to the separate docs repo per project conventions and removed in-repo docs from this PR. Docs PR: langchain-ai/docs#1455 |
91116e2 to
bb2bb22
Compare
Summary
Testing
Fixes #33885
Refs agyn-sandbox#1