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 tuning/config/configs unit tests #85

Closed
wants to merge 1 commit into from
Closed

add tuning/config/configs unit tests #85

wants to merge 1 commit into from

Conversation

tedhtchang
Copy link
Collaborator

Description of the change

Adds unit tests for tuning.config.configs modules

Related issue number

Closes #72

How to verify the PR

git clone https://github.com/tedhtchang/fms-hf-tuning.git -b add-configs-unit-tests
cd ms-hf-tuning
pytest tests/configs/test_configs.py

Was the PR tested

  • I have added >=1 unit test(s) for every new method I have added.
  • I have ensured all unit tests pass

Copy link
Collaborator

@tharapalanivel tharapalanivel left a comment

Choose a reason for hiding this comment

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

This is great, thanks for adding unit tests for the configs Ted! I left a few comments, can we also add a few more tests for edge cases based on maybe input value/datatype validation if possible? Thank you!

tests/configs/test_configs.py Outdated Show resolved Hide resolved
tests/configs/test_configs.py Outdated Show resolved Hide resolved
tests/configs/test_configs.py Outdated Show resolved Hide resolved
tests/configs/test_configs.py Outdated Show resolved Hide resolved
tests/configs/test_configs.py Outdated Show resolved Hide resolved
@tedhtchang
Copy link
Collaborator Author

@tharapalanivel I realized your PR#83 already test the configs module. I should close this PR unless you have examples of edges configs you want me to test.



def test_model_argument_configs_default():
data_arguments = tuning_config.DataArguments
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can you add docstrings to these tests describing their goal and scope

@anhuong
Copy link
Collaborator

anhuong commented May 10, 2024

@tedhtchang can we close this PR if it has duplicative unit tests?

@tedhtchang tedhtchang closed this May 10, 2024
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.

Add unit tests for tuning/config/configs.py
4 participants