Add learn2ask example
#356
Conversation
Summary of ChangesHello @chenyushuo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request adds a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new learn2ask example, including data preparation scripts, workflow logic, prompts, and configuration. The implementation is comprehensive, but there are several areas for improvement. I've identified a critical bug in the evaluation script that will cause a KeyError, and some high-severity issues related to data processing robustness, scalability, and use of global variables in the workflow. I've also included several medium-severity suggestions to improve code quality, maintainability, and correctness across the new files. My feedback includes code suggestions to address these points.
| for index, sample in enumerate(sample_list[:700]): | ||
| record = copy.deepcopy(sample) | ||
| print(f"index: {index}, session_id: {sample['session_id']}") | ||
| user_content = "# 对话记录\n" + sample["input"] |
There was a problem hiding this comment.
This line will raise a KeyError because sample['input'] does not exist in the data generated by the previous script (2_build_dataset.py). The conversation history should be constructed from sample['messages']. You can use the msg2str helper function for this, but it needs to be moved to the module scope to be accessible here.
| user_content = "# 对话记录\n" + sample["input"] | |
| user_content = "# 对话记录\n" + msg2str(sample["messages"]) |
| def main(input_file_path, output_file_path): | ||
| with open(input_file_path, "r") as lines: | ||
| data_list = [json.loads(line.strip()) for line in lines] | ||
| print("data loaded to a list...") | ||
|
|
||
| for data in data_list: | ||
| if_keep, info_set, decision = process_message(data) | ||
| if not if_keep: | ||
| continue | ||
| new_item = { | ||
| "cid": data["cid"], | ||
| "session_id": data["session_id"], | ||
| "diagn": data["diagn"], | ||
| "messages": data["messages"], | ||
| "decision_truth": decision, | ||
| "info_truth": info_set, | ||
| } | ||
| with open(output_file_path, "a") as f: | ||
| f.write(json.dumps(new_item, ensure_ascii=False) + "\n") | ||
| print("job done!") |
There was a problem hiding this comment.
The current implementation loads the entire input file into memory, which is not scalable and may cause a MemoryError with large datasets. Additionally, the output file is opened and closed for every line written, which is inefficient. The process should be changed to read and write line-by-line to handle large files efficiently.
def main(input_file_path, output_file_path):
with open(input_file_path, "r", encoding="utf-8") as infile, open(
output_file_path, "w", encoding="utf-8"
) as outfile:
print("data processing started...")
for line in infile:
data = json.loads(line.strip())
if_keep, info_set, decision = process_message(data)
if not if_keep:
continue
new_item = {
"cid": data["cid"],
"session_id": data["session_id"],
"diagn": data["diagn"],
"messages": data["messages"],
"decision_truth": decision,
"info_truth": info_set,
}
outfile.write(json.dumps(new_item, ensure_ascii=False) + "\n")
print("job done!")
examples/learn2ask/README.md
Outdated
| --- | ||
|
|
||
| ## Step 3. Evaluation | ||
| > 📁 The evaluation pipeline scripts are provided in the `ckpt_evaluation/` folder. |
There was a problem hiding this comment.
The path mentioned for evaluation scripts, ckpt_evaluation/, is incorrect. The evaluation script 3_rollout_then_evaluate.py is located in the examples/learn2ask/data_prepare/ directory. This should be corrected to avoid confusion for users trying to follow the instructions.
| > 📁 The evaluation pipeline scripts are provided in the `ckpt_evaluation/` folder. | |
| > 📁 The evaluation pipeline scripts are provided in the `examples/learn2ask/data_prepare/` folder. |
| train_mode = "Ra+Rs" | ||
| fusion_mode = "default" |
There was a problem hiding this comment.
Using global variables train_mode and fusion_mode to control the workflow's behavior is not good practice. It makes the code harder to understand, maintain, and test, as the behavior can be altered from anywhere. These settings should be passed as configuration parameters through the Task object's workflow_args and accessed via self.task.workflow_args.
| if remaining_messages: | ||
| remaining_chat_parts = [] | ||
| for msg in remaining_messages: | ||
| role = msg.get("role", "") | ||
| content = msg.get("content", "") | ||
| remaining_chat_parts.append(f"{role}: {content}") | ||
| round_entry["remaining_chat"] = "\n".join(remaining_chat_parts) | ||
| else: | ||
| round_entry["remaining_chat"] = "" |
There was a problem hiding this comment.
Converting remaining_chat to a single string causes a loss of structured information, which makes downstream processing fragile (e.g., in 2_build_dataset.py). It's more robust to keep remaining_chat as a list of message dictionaries. This change will require small adaptations in the consumer scripts but will significantly improve the data pipeline's reliability.
if remaining_messages:
round_entry["remaining_chat"] = remaining_messages
else:
round_entry["remaining_chat"] = []| try: | ||
| format_score = float(res_dict.get("format_score", 0.0)) | ||
| content_score = float(res_dict.get("content_score", 0.0)) | ||
| res_think = res_dict.get("think", "无") | ||
| except Exception as e: | ||
| print(e) |
There was a problem hiding this comment.
Catching a generic Exception and just printing it can hide bugs and makes debugging difficult. The exception handling should be more specific (e.g., ValueError, TypeError from the float() conversion) and log the error with more context for better diagnostics.
| test_file_path = "path/to/your/input_file.jsonl" # <<< Your test sample path | ||
| rollout_file_path = "path/to/your/rollout_file.jsonl" # <<< rollout results given test samples | ||
| eval_model_path = "path/to/your/ckpt/or/model" # <<< ckpt for testing | ||
| grader_model_path = "path/to/your/qwen2.5-32b-instruct" # <<< model to empower the grading | ||
| eval_file_path = ( | ||
| "path/to/your/rollout_eval_result_file.jsonl" # <<< final output given rollout results | ||
| ) |
There was a problem hiding this comment.
The file paths for test data, models, and outputs are hardcoded. This makes the script inflexible. These should be passed as command-line arguments using a library like argparse to make the script more reusable.
| if not model_path: | ||
| return "Error: model_path is required for local vLLM inference" |
There was a problem hiding this comment.
Returning an error message as a string when model_path is missing is not ideal. The calling function expects a list-like string from the LLM and will fail while parsing this error string. It's better to raise a ValueError to provide a clear and immediate error signal.
| if not model_path: | |
| return "Error: model_path is required for local vLLM inference" | |
| if not model_path: | |
| raise ValueError("model_path is required for local vLLM inference") |
| # Read and process each session | ||
| with open(input_file, "r", encoding="utf-8") as infile: | ||
| for line_num, line in enumerate(infile, 1): | ||
| if line.strip(): | ||
| try: | ||
| session = json.loads(line) | ||
| print( | ||
| f"Processing session {session.get('session_id', 'unknown')} (line {line_num})..." | ||
| ) | ||
|
|
||
| # Process the session | ||
| processed_lines = process_session( | ||
| session, model_call_mode, max_retries, **kwargs | ||
| ) | ||
| for line in processed_lines: | ||
| with open(output_file, "a", encoding="utf-8") as outfile: | ||
| outfile.write(line + "\n") |
There was a problem hiding this comment.
The output file is opened in append mode ("a") within the loop for every processed line. This is inefficient as it involves repeated file open/close operations. It's better to open the output file once before the loop starts.
with open(input_file, "r", encoding="utf-8") as infile, open(output_file, "w", encoding="utf-8") as outfile:
for line_num, line in enumerate(infile, 1):
if line.strip():
try:
session = json.loads(line)
print(
f"Processing session {session.get('session_id', 'unknown')} (line {line_num})..."
)
# Process the session
processed_lines = process_session(
session, model_call_mode, max_retries, **kwargs
)
for processed_line in processed_lines:
outfile.write(processed_line + "\n")| """ | ||
| Process all sessions in a JSONL file and save results based on specified output mode. | ||
|
|
||
| Args: | ||
| input_file (str): Path to input JSONL file | ||
| output_mode (str): Either "single_file" or "multiple_files" | ||
| output_file (str): Path to output file (required if output_mode="single_file") | ||
| output_dir (str): Path to output directory (required if output_mode="multiple_files") | ||
| model_call_mode (str): Either "online_api" or "local_vllm" | ||
| max_retries (int): Maximum number of retries for LLM calls | ||
| **kwargs: Additional parameters for API calls | ||
|
|
||
| Returns: | ||
| str: Success message or error information | ||
| """ |
There was a problem hiding this comment.
The docstring for process_jsonl_file is out of sync with the function's signature. It mentions output_mode and output_dir as arguments, but they are not defined or used in the function. The docstring should be updated to accurately reflect the function's parameters.
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
There was a problem hiding this comment.
rename the dir plugins to app or workflow
| algorithm: | ||
| algorithm_type: grpo | ||
| repeat_times: 5 | ||
| sample_strategy: warmup |
| experience_buffer: | ||
| name: experience_buffer | ||
| storage_type: queue | ||
| enable_progress_bar: false |
There was a problem hiding this comment.
default values could be removed
3 participants
Description
As the title says.
Checklist
Please check the following items before code is ready to be reviewed.