search

stoyan-stoyanov / llmflows

LLMFlows - Simple, Explicit and Transparent LLM Apps
https://llmflows.readthedocs.io
MIT License
668 stars 34 forks source link

FunctionalFlowStep #31

Open OlaRonning opened 1 year ago

OlaRonning commented 1 year ago

Why not use **kwargs here?

https://github.com/stoyan-stoyanov/llmflows/blob/3c4ceb7964dd85d0ab59f7865aa37337201454fa/llmflows/flows/functional_flowstep.py#L42

Then you can name keyword args to the function (i.e., generate), and the remaining will be the arguments for flowstep_fn, so this will go away

https://github.com/stoyan-stoyanov/llmflows/blob/3c4ceb7964dd85d0ab59f7865aa37337201454fa/llmflows/flows/functional_flowstep.py#L58-L60

Rename flowstep_fn to inner_fn or step_fn; it's part of a flowstep class, so the context is already clear.

https://github.com/stoyan-stoyanov/llmflows/blob/3c4ceb7964dd85d0ab59f7865aa37337201454fa/llmflows/flows/functional_flowstep.py#L39

^ is not used.

stoyan-stoyanov commented 1 year ago

Good point. Actually, the generate() method is supposed to be used only by the flow class so it might be also _generate() And looking at it the return type is too general.

Also, the flow class should be already taking care of passing the right inputs so I don't know why I added the filtered_inputs check.

required_keys is used in the flow class: https://github.com/stoyan-stoyanov/llmflows/blob/3c4ceb7964dd85d0ab59f7865aa37337201454fa/llmflows/flows/flow.py#L76

OlaRonning commented 1 year ago

If the two are that tightly interlinked, then you should have a runtime check that you have a flowstep instance.