[BFCL] The most unreadable codes I have ever seen

[sjcfr]

Can you guys clean up this messy code? Even vibe coding would be better.

[HuanzhiMao]

Hi @sjcfr,
Thanks for raising this.
To help us act on it, please include: file(s)/function(s), examples of what’s hard to follow, and concrete suggestions (naming, structure, comments, tests, formatting).
We’re happy to improve readability, we just need actionable details.

[xtknight]

Unfortunately on an objective level I have to agree with this. I really appreciate the effort on the project, but it would be helpful to have better variable names and a more clear flow. I don't mean to nitpick, but I'm trying to provide some constructive examples.

1. gorilla/berkeley-function-call-leaderboard/bfcl_eval/model_handler/base_handler.py

   Lines 261 to 293 in 6ea5797

   	try:
   	decoded_model_responses = self.decode_execute(
   	model_responses, has_tool_call_tag=False
   	)
   	current_step_inference_log.append(
   	{
   	"role": "handler_log",
   	"content": "Successfully decoded model response.",
   	"model_response_decoded": decoded_model_responses,
   	}
   	)
   	if is_empty_execute_response(decoded_model_responses):
   	print("Empty response from the model. Proceed to next turn.")
   	current_step_inference_log.append(
   	{
   	"role": "handler_log",
   	"content": f"Empty response from the model. Proceed to next turn.",
   	"model_response_decoded": decoded_model_responses,
   	}
   	)
   	break
   	except Exception as e:
   	print("Failed to decode the model response. Proceed to next turn.")
   	current_step_inference_log.append(
   	{
   	"role": "handler_log",
   	"content": f"Error decoding the model response. Proceed to next turn.",
   	"error": str(e),
   	}
   	)
   	break

Here I see "Successfully decoded model response" and then a check for an "empty response" which seem contradictory to me. When I saw a message saying it was successfully decoded and then that it was empty, I wasn't sure whether to think it was working as expected or not. Furthermore, if the model response decode fails, it doesn't make sense to me why it would go on with another turn of conversation. It seems at that point that errors would just propagate and it wouldn't be worth it to continue.

2. gorilla/berkeley-function-call-leaderboard/bfcl_eval/model_handler/base_handler.py

   Lines 325 to 330 in 6ea5797

   	current_step_inference_log.append(
   	{
   	"role": "handler_log",
   	"content": f"Model has been forced to quit after {MAXIMUM_STEP_LIMIT} steps.",
   	}
   	)

Inserting "handler_log" roles into a structure schema that generally only accepts roles such as "system", "user", "assistant" makes this very difficult to read and difficult to copy the structure to use it for debugging elsewhere.

3. gorilla/berkeley-function-call-leaderboard/bfcl_eval/model_handler/base_handler.py

   Line 503 in 6ea5797

   while True:

The "while True" makes it difficult to see what the point of this loop is and when it ever ends. Although it's not the "while True" that's the main issue, it's just that I can't figure out what the loop is actually doing or what a "step" means.

4. Having different _FC and _prompting functions for everything makes it difficult to see the flow. I'm not sure why it was necessary to split these and duplicate the code when a branch between FC and prompting would probably suffice inside a single function. I feel like these should be abstracted into a class which has an inference function, and then override the needed parts for FC models and for prompting models as needed, while also calling common ones through the parent class.

5. gorilla/berkeley-function-call-leaderboard/bfcl_eval/model_handler/base_handler.py

   Lines 762 to 766 in 6ea5797

   	def decode_execute(self, result, has_tool_call_tag: bool):
   	"""
   	This method takes raw model output (from `_parse_query_response_xxx`) and convert it to standard execute checker input.
   	"""
   	raise NotImplementedError

decode_execute: this makes it sound like it's decoding and executing something, but in reality, it's just decoding as far as I can tell. I do understand this function's purpose is commented, but it doesn't help when reading a long function and thinking execution is actually happening when it isn't.

6. There's a general lack of class organization, and instead many many list variables inside many different functions, making the code appear very long. More classes with clear purpose, less long strings of code.

gorilla/berkeley-function-call-leaderboard/bfcl_eval/eval_checker/eval_runner.py

Lines 218 to 236 in 6ea5797

	multi_turn_model_result_list_decoded: list[list[list[str]]] = []
	# Try decoding the model results into executable function calls
	for single_turn_model_result_list in model_result_list:
	single_turn_model_result_list_decoded = []
	for model_result_item in single_turn_model_result_list:
	# model_result_item is per step
	try:
	decoded_result: list[str] = handler.decode_execute(
	model_result_item, has_tool_call_tag=False
	)
	if is_empty_execute_response(decoded_result):
	# Empty output is not considered as a valid function call
	continue
	single_turn_model_result_list_decoded.append(decoded_result)
	except Exception as e:
	# Ignore any failed decoding and continue to the next message
	# We only care about the decoded function call, not the error message or if the model is chatting
	continue
	multi_turn_model_result_list_decoded.append(single_turn_model_result_list_decoded)

Three nested lists, and from an outsider's perspective, it requires extensive analysis of the code to figure out what each dimension means or what the point of this loop is.

How about something like this? (It was made by GPT, so I'm not sure if it's right or not, but even though it's longer it looks readable.)

from typing import List

@dataclass
class DecodedStep:
    calls: List[str]

@dataclass
class DecodedTurn:
    steps: List[DecodedStep] = field(default_factory=list)

    def add_step(self, step: DecodedStep) -> None:
        self.steps.append(step)

@dataclass
class DecodedConversation:
    turns: List[DecodedTurn] = field(default_factory=list)

    def add_turn(self, turn: DecodedTurn) -> None:
        self.turns.append(turn)

def decode_conversation(model_result_list, handler) -> DecodedConversation:
    conversation = DecodedConversation()

    for turn_results in model_result_list:
        turn = DecodedTurn()

        for step_result in turn_results:
            try:
                decoded = handler.decode_execute(
                    step_result,
                    has_tool_call_tag=False,
                )
            except Exception:
                continue

            if is_empty_execute_response(decoded):
                continue

            step = DecodedStep(calls=decoded)
            turn.add_step(step)

        conversation.add_turn(turn)

    return conversation

7. It's not clear where the "bfcl generate" part of the code is and where the "bfcl eval" part is. "eval" seems to be in "eval_checker", but "generate" seems to be somewhere among the base classes at a different level.

8. The extensive dependencies among the test data make it extremely difficult to debug, so it would be useful to have unit tests that we can use when adding new models or debugging a problem. Running the entire set just to make sure the prerequisites were run means waiting for inference every time to debug a small issue. (Or if there is one, maybe I haven't been able to find it?)

9. The widespread use of continue and breaks inside nested for/while loops makes it very difficult to see the flow. Use of exceptions with clear names would be nice.

10. The eval_checker is difficult to understand. For example, is AST evaluation happening in eval_runner.py, or in ast_eval/ast_checker.py? The filenames would indicate I'd expect AST evaluation code in ast_checker.py, but in reality, there's also extensive code regarding AST evaluation in eval_runner.py. It seems as though these should all be organized into classes and there should be less code for each individual evaluator in the common part (eval_runner.py).

gorilla/berkeley-function-call-leaderboard/bfcl_eval/eval_checker/eval_runner.py

Lines 613 to 664 in 6ea5797

	def ast_file_runner(
	handler: BaseHandler,
	model_result,
	prompt,
	possible_answer,
	test_category,
	model_name,
	score_dir,
	):
	assert (
	len(model_result) == len(prompt) == len(possible_answer)
	), f"The length of the model result ({len(model_result)}) does not match the length of the prompt ({len(prompt)}) or possible answer ({len(possible_answer)}). Please check the input files for completeness."
	if is_java(test_category):
	language = Language.JAVA
	return_format = ReturnFormat.JAVA
	elif is_js(test_category):
	language = Language.JAVASCRIPT
	return_format = ReturnFormat.JAVASCRIPT
	else:
	language = Language.PYTHON
	return_format = ReturnFormat.PYTHON
	result = []
	correct_count = 0
	for i in range(len(model_result)):
	index = model_result[i]["id"]
	model_result_item = model_result[i]["result"]
	prompt_entry = prompt[i]
	possible_answer_item = possible_answer[i]["ground_truth"]
	entry_result = _evaluate_single_ast_entry(
	handler,
	index,
	model_result_item,
	possible_answer_item,
	prompt_entry,
	model_name,
	test_category,
	language=language,
	return_format=return_format,
	has_tool_call_tag=False,
	)
	if entry_result["valid"]:
	correct_count += 1
	else:
	result.append(entry_result)
	return save_eval_results(
	result, correct_count, model_result, test_category, model_name, score_dir
	)

(Even "_evaluate_single_ast_entry" is in eval_runner.py as well.)

The following code is duplicated three (the loop itself, five) times in this py file, and it seems like it could be broken out into a common evaluation function instead of repeated for each evaluation type.

    for i in range(len(model_result)):
        index = model_result[i]["id"]
        model_result_item = model_result[i]["result"]
        prompt_entry = prompt[i]

11. Rather than calling /v1/completions and reimplementing every single model's template one-by-one, it seems it would be far easier to call /v1/chat/completions and utilize the widely used OpenAI-compatible template that almost every model and server supports these days.

I hope this helps to improve the readability. I think the test set is well made, and if the test harness itself was more readable, this would be a much easier project to contribute to and an easier benchmark to run. Perhaps this could be considered for the next version.