Skip to content

Conversation

@tharapalanivel
Copy link
Collaborator

Description of the change

Add logging and tests for run_quant.py

Related issue number

How to verify the PR

All unit tests pass

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

Signed-off-by: Thara Palanivel <[email protected]>
Signed-off-by: Thara Palanivel <[email protected]>
Signed-off-by: Thara Palanivel <[email protected]>
Signed-off-by: Thara Palanivel <[email protected]>
Signed-off-by: Thara Palanivel <[email protected]>
Signed-off-by: Thara Palanivel <[email protected]>
Signed-off-by: Thara Palanivel <[email protected]>
Signed-off-by: Thara Palanivel <[email protected]>
Signed-off-by: Thara Palanivel <[email protected]>
Copy link
Collaborator

@BrandonGroth BrandonGroth left a comment

Choose a reason for hiding this comment

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

Some testing comments:

  • Can we put the entire run_quant test in its own tests/run_quant directory?
  • We should be making fixtures of any testing inputs like MODEL_ARGS, etc.. There likely will be a time where multiple different args will be accepted, so its good to make them lists and parameterize them (even if single-valued for now). This registers them in Pytest and makes it easier to see errors, rerun single failed test cases, etc.. See the models/conftest.py examples.

Signed-off-by: Thara Palanivel <[email protected]>
@tharapalanivel
Copy link
Collaborator Author

Some testing comments:

  • Can we put the entire run_quant test in its own tests/run_quant directory?
  • We should be making fixtures of any testing inputs like MODEL_ARGS, etc.. There likely will be a time where multiple different args will be accepted, so its good to make them lists and parameterize them (even if single-valued for now). This registers them in Pytest and makes it easier to see errors, rerun single failed test cases, etc.. See the models/conftest.py examples.

@BrandonGroth Typically the same dir structure as src is followed for tests, is there a specific reason that we want a new directory for a single script? Utilizing fixtures for input args is a great point, I've got a follow-up incoming PR for building docker images that has similar tests so I'll modify them all at once and create a new PR for that next week.

@chichun-charlie-liu chichun-charlie-liu merged commit 1e7856e into foundation-model-stack:main Jan 13, 2025
10 checks passed
@tharapalanivel tharapalanivel deleted the logging branch January 23, 2025 05:44
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.

3 participants