-
Notifications
You must be signed in to change notification settings - Fork 19
Improve run_api
dynamic schema creation
#110
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: main
Are you sure you want to change the base?
Conversation
@@ -270,8 +279,9 @@ async def run_endpoint_without_files(run_req: request_model) -> response_model: | |||
def add_pipeline_api_route(app: FastAPI, pipeline_name: str, pipeline_wrapper: BasePipelineWrapper) -> None: | |||
clog = log.bind(pipeline_name=pipeline_name) | |||
|
|||
RunRequest = create_request_model_from_callable(pipeline_wrapper.run_api, f'{pipeline_name}Run') | |||
RunResponse = create_response_model_from_callable(pipeline_wrapper.run_api, f'{pipeline_name}Run') | |||
docstring = docstring_parser.parse(inspect.getdoc(pipeline_wrapper.run_api) or "") |
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.
I would expect more failures with docstring parsing than with the previous simple approach.
What do you think?
- Do we want to force users to stick to this more rigid format?
- Should we instead provide a safe fallback if docstring parsing fails?
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.
I think your suggestion makes sense, but honestly, I haven't been able to make docstring.parse()
raise an exception in my tests 😅.
Even when mixing different docstring formats (or not strictly adhering to them), it doesn't raise any exceptions. Instead, it just skips certain parameters. If, for example, you type ((int)
, you might get a slightly malformed type_name
value, but the function doesn't fail -it simply behaves unexpectedly. Evaluating this behaviour automatically could be tricky.
I also noticed that within Haystack, .parse()
exceptions are not explicitly handled either (in ComponentTool
).
Perhaps we can leave the code as it is for now unless a bug pops out in the future?
I am also attaching an example: if you provide a def run_api(self, urls: List[str], question: str) -> str:
"""
Ask a question to a list of websites.
Args:
urls (List[str]): List of URLs to ask the question to.
question (str): Question to ask the websites.
Returns:
str: Answer to the question.
"""
result = self.pipeline.run({"fetcher": {"urls": urls}, "prompt": {"query": question}})
return result["llm"]["replies"][0] You'll get a Tool description like the following (using MCP Inspector): |
This will add better description of
run_api
params / return type to dynamically create request / response pydantic models using docstring_parser to automatically parserun_api
docstring.For example, if you have a
run_api
like:It will add proper description, defaults (if present) and type to each param on pydantic model schema for
question
,urls
and the return type.This improved description will improve Hayhooks usage as OpenAPI Tool or MCP Tool.