Skip to content
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

Update save_conversations tool #1421

Merged
merged 5 commits into from
Feb 11, 2025
Merged

Update save_conversations tool #1421

merged 5 commits into from
Feb 11, 2025

Conversation

xrdaukar
Copy link
Collaborator

Description

-- Add flags to split multi-turn conversations into N single-turn conversations
-- Modify Conversation.filter_messages() method to accept user-defined predicate.

Related issues

Towards OPE-355

Before submitting

  • This PR only changes documentation. (You can ignore the following checks in that case)
  • Did you read the contributor guideline Pull Request guidelines?
  • Did you link the issue(s) related to this PR in the section above?
  • Did you add / update tests where needed?

Reviewers

At least one review from a member of oumi-ai/oumi-staff is required.

@xrdaukar xrdaukar marked this pull request as ready for review February 11, 2025 02:21
@@ -65,13 +66,83 @@ def _load_sft_dataset(
return dataset


def _split_message_list_on_user(messages: list[Message]) -> list[list[Message]]:
turn_start_indices: list[int] = []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Food for thought here: should you add a check for an empty input? I think _split_message_list_on_user([]) should return [] not [ [] ]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, updated!

"""Gets all messages in the conversation, optionally filtered by role.

Args:
role: The role to filter messages by.
If None, returns all messages.
If None, no filtering by role is applied.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implies that the filter_fn has to use to role field, which isn't necessarily true.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added Optional to make it more clear

@xrdaukar xrdaukar merged commit aa6ae8f into main Feb 11, 2025
2 checks passed
@xrdaukar xrdaukar deleted the xrdaukar/ask-gen branch February 11, 2025 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants