Open KatoStevenMubiru opened 1 month ago
Will do! Great suggestion. I'll also try to maybe add a separate tool to set up the config files, so it's displayed in the UI as well.
Awesome, thanks Kacper! 👍 The idea of a separate tool for setting up the config files sounds great.
Awesome, thanks Kacper! 👍 The idea of a separate tool for setting up the config files sounds great.
Descriptive names are already there, only the tool to set up the configs needs to be added.
quality: str = config.get("quality", "1")
cost: str = config.get("cost", "4.65e-03")
time_to_first_token: str = config.get("time_to_first_token", "2.08e-05")
inter_token_latency: str = config.get("inter_token_latency", "2.07e-03")
endpoint: Union[list[str], str] = config.get("endpoint", None)
model: Union[list[str], str] = config.get("model", None)
provider: Union[list[str], str] = config.get("provider", None)
router: str = f"router@q:{quality}|c:{cost}|t:{time_to_first_token}|i:{inter_token_latency}"
Thanks for updating the docstring! For the router
string, I was referring to those variables within the string itself (q:{quality}
, c:{cost}
, etc.). It would be great to replace those with more descriptive names like quality_weight
, cost_weight
, etc.
Also, have you had a chance to consider adding input validation to the config
dictionary?
I've included a code example below to illustrate these suggestions, but I won't be pushing these changes to the main branch yet.
from typing import Optional, Sequence, Union
from unify import Unify
from promptflow.core import tool
@tool
def optimize_llm(connection: Unify, config: Optional[dict], input_text: Union[str, Sequence] = " ") -> Union[dict, str]:
"""
Selects the optimal model for a step of a flow.
# ... (Rest of docstring) ...
"""
assert isinstance(connection.get_credit_balance(), (str, float, int))
if not isinstance(config, dict):
config = {}
# Input validation
def validate_weight(value, name):
try:
float(value)
except ValueError:
raise ValueError(f"{name} must be a valid float string")
quality_weight: str = config.get("quality_weight", "1")
cost_weight: str = config.get("cost_weight", "4.65e-03")
# ... (Rest of the function) ...
Let me know what you think about these ideas! I'm happy to discuss them further.
Thanks again!
Thanks for updating the docstring! For the
router
string, I was referring to those variables within the string itself (q:{quality}
,c:{cost}
, etc.). It would be great to replace those with more descriptive names likequality_weight
,cost_weight
, etc.Also, have you had a chance to consider adding input validation to the
config
dictionary?I've included a code example below to illustrate these suggestions, but I won't be pushing these changes to the main branch yet.
from typing import Optional, Sequence, Union from unify import Unify from promptflow.core import tool @tool def optimize_llm(connection: Unify, config: Optional[dict], input_text: Union[str, Sequence] = " ") -> Union[dict, str]: """ Selects the optimal model for a step of a flow. # ... (Rest of docstring) ... """ assert isinstance(connection.get_credit_balance(), (str, float, int)) if not isinstance(config, dict): config = {} # Input validation def validate_weight(value, name): try: float(value) except ValueError: raise ValueError(f"{name} must be a valid float string") quality_weight: str = config.get("quality_weight", "1") cost_weight: str = config.get("cost_weight", "4.65e-03") # ... (Rest of the function) ...
Ok, I'll use that as an opportunity to separate config preparation as an additional tool then.
Alright then
After looking at the optimize_llm tool in
optimize_llm_tool.py
and had a few thoughts on how we could make it even better. Nothing major, just some small tweaks to potentially enhance clarity and usability. 😊Docstring Clarity: The current docstring is good, but I was wondering if we could expand it a bit to give more details about the config options. It would be really helpful to have clear explanations for each parameter (like "quality," "cost," etc.), including what kind of values they expect and what those values represent.
For example, it would be great to specify the units for "time_to_first_token" (seconds?) and what the different quality levels ("1", "2", "3") actually mean.
Maybe something like this?
Variable Names: Just a small thought—would it be possible to use more descriptive variable names for those single-letter variables (q, c, t, i) in the router string? Something like quality_weight, cost_weight, etc. might make the code a bit easier to follow. 😊
Input Validation: It might be a good idea to add some checks to make sure the values provided in the config dictionary are valid. This could help prevent unexpected issues or errors if someone accidentally enters the wrong type of value.
I think these small changes could make the optimize_llm tool even clearer and more user-friendly. What do you think? 🤔