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

add helper to verify the correct model output file exists #2245

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

winglian
Copy link
Collaborator

@winglian winglian commented Jan 9, 2025

Description

Adds a helper function to check for the output model when doing e2e training tests.

Motivation and Context

It can be cumbersome to remember which file to look for when adding the check to tests, so this handles all that logic.

How has this been tested?

update tests!

Screenshots (if appropriate)

Types of changes

Social Handles (Optional)

@winglian winglian force-pushed the helper-model-output-exists branch 2 times, most recently from 0beb9ac to 1d9d237 Compare January 9, 2025 04:54
@NanoCode012
Copy link
Collaborator

I saw that you didn't change all files in e2e like test_dpo.py. Even though they had assert (Path(temp_dir) / "checkpoint-20/adapter_model.safetensors").exists()

Should this be changed to use your new function but pass Path(temp_dir) / "checkpoint-20" as input path instead?

@winglian winglian force-pushed the helper-model-output-exists branch from f80abe4 to 4067c50 Compare January 10, 2025 13:54
Copy link
Contributor

@djsaunde djsaunde left a comment

Choose a reason for hiding this comment

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

Just a nit.

import pytest
from e2e.utils import check_model_output_exists
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: something weird is going on here with the sort order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants