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.

Sign up for GitHub

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

Jump to bottom

added data processing logging #119

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

added data processing logging #119

wants to merge 1 commit into from

Conversation

aldopareja
Copy link
Member

No description provided.

JamesKunstle
JamesKunstle approved these changes Jul 5, 2024
View reviewed changes
Copy link
Contributor

@JamesKunstle JamesKunstle left a comment

Choose a reason for hiding this comment

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

lgtm, one question to consider but it can go in

src/instructlab/training/data_process.py
@@ -193,6 +194,7 @@ def remove_pretrain_system_messages(example: dict):


def main(args: DataProcessArgs):
print(json.dumps(args.__dict__))
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have a logger we could use for this instead of printing directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

we could have a logger but it's used mostly on the training repo.

@aldopareja aldopareja force-pushed the ap/print_data_args branch from fa75b8d to 9a4f0d4 Compare July 16, 2024 03:23
@JamesKunstle
Copy link
Contributor

@aldo-pareja this can go in if you fix linting/DCO/rebase.

RobotSail
RobotSail approved these changes Oct 4, 2024
View reviewed changes
Copy link
Member

@RobotSail RobotSail left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 4, 2024

This pull request has merge conflicts that must be resolved before it can be
merged. @aldopareja please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot removed the needs-rebase label Dec 17, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 17, 2024

This pull request has merge conflicts that must be resolved before it can be
merged. @aldopareja please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JamesKunstle JamesKunstle JamesKunstle approved these changes

@RobotSail RobotSail RobotSail approved these changes

Assignees
No one assigned
Labels
ci-failure needs-rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aldopareja @JamesKunstle @RobotSail @aldo-pareja