Skip to content
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

feat: Add variable to specify inputs as optional to ConditionalRouter #8568

Merged
merged 7 commits into from
Nov 26, 2024

Conversation

vblagoje
Copy link
Member

@vblagoje vblagoje commented Nov 22, 2024

Why:

Enhances the ConditionalRouter component by adding support for optional parameters, enabling default/fallback routing behavior when certain inputs are not provided at runtime.

What:

Introduces optional_variables parameter to specify which route variables can be omitted
Improves documentation with clear examples of fallback routing patterns
Implements proper serialization/deserialization for the new field

How can it be used:

Define routes with fallback behavior when certain parameters are missing:

from haystack import Pipeline
from haystack.components.routers import ConditionalRouter

routes = [
    {
        "condition": '{{ path == "rag" }}',
        "output": "{{ question }}",
        "output_name": "rag_route",
        "output_type": str
    },
    {
        "condition": "{{ True }}",  # fallback route
        "output": "{{ question }}",
        "output_name": "default_route",
        "output_type": str
    }
]

router = ConditionalRouter(routes, optional_variables=["path"])
pipe = Pipeline()
pipe.add_component("router", router)

# When 'path' is provided in the pipeline:
result = pipe.run(data={"router": {"question": "What?", "path": "rag"}})
assert result["router"] == {"rag_route": "What?"}

# When 'path' is not provided, fallback route is taken:
result = pipe.run(data={"router": {"question": "What?"}})
assert result["router"] == {"default_route": "What?"}

How did you test it:

Unit tests verify both direct component usage and pipeline integration
Tests cover simple and complex scenarios with and without optional parameters

@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels Nov 22, 2024
@coveralls
Copy link
Collaborator

coveralls commented Nov 22, 2024

Pull Request Test Coverage Report for Build 11974160625

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 18 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.04%) to 90.285%

Files with Coverage Reduction New Missed Lines %
components/routers/conditional_router.py 18 86.15%
Totals Coverage Status
Change from base Build 11972705853: 0.04%
Covered Lines: 7881
Relevant Lines: 8729

💛 - Coveralls

@vblagoje vblagoje marked this pull request as ready for review November 22, 2024 13:14
@vblagoje vblagoje requested review from a team as code owners November 22, 2024 13:14
@vblagoje vblagoje requested review from dfokina, anakin87, silvanocerza and sjrl and removed request for a team and anakin87 November 22, 2024 13:14
@vblagoje
Copy link
Member Author

@silvanocerza @sjrl - if I understood correctly the originating issue this seems like a relatively simple fix. Added a bunch of tests, maybe a bit more than needed just to make sure as this is an important component.

Comment on lines 164 to 170
# When 'path' is provided, specific route is taken:
result = router.run(question="What?", path="rag")
assert result == {"rag_route": "What?"}

# When 'path' is not provided, fallback route is taken:
result = router.run(question="What?")
assert result == {"default_route": "What?"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Good example! Except this example actually works pre your changes. To get the error we were originally seeing you would need to put it in a Pipeline first. Even a single component pipeline would do. Otherwise it's a bit misleading here since this example would actually work without specifying the optional_variables parameter.

Comment on lines 214 to 218
warn(
f"The following optional variables are specified but not used in any route: {unused_optional_vars}. "
"Check if there's a typo in variable names.",
UserWarning,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use the logger.warning here instead or in addition to? Using the logger allows us to surface these warnings to users in dC more easily.

@sjrl
Copy link
Contributor

sjrl commented Nov 22, 2024

Yeah this looks like it solves the issue thanks @vblagoje !

@vblagoje vblagoje added this to the 2.8.0 milestone Nov 22, 2024
@vblagoje vblagoje merged commit 59f1e18 into main Nov 26, 2024
18 checks passed
@vblagoje vblagoje deleted the cr_optional_vars branch November 26, 2024 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:tests type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add variable to specify inputs as optional to ConditionalRouter
4 participants