Unit tests by tharapalanivel · Pull Request #83 · foundation-model-stack/fms-hf-tuning

tharapalanivel · 2024-03-07T18:39:17Z

Description of the change

Adding unit tests for pt and lora tuning method using dummy model, edge cases, invalid requests, etc.

Cont. of PR #79

Related issue number

Closes #74

How to verify the PR

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 <130496890+tharapalanivel@users.noreply.github.com>

Signed-off-by: Anh-Uong <anh.uong@ibm.com>

Ssukriti · 2024-03-08T23:32:04Z

This PR looks good to me. Before we add edge cases, @tharapalanivel can we also add a unit test for fine tuning? there will not be any peft type associated with it
We can also merge the PR once ft is added and reviewed with the status at the time. Any other edge cases can go in subsequent PRs as well

Ssukriti · 2024-03-09T00:44:24Z

 accelerate>=0.20.3
 packaging
-transformers>=4.34.1
+transformers>=4.34.1,<4.38.0


#53 is merged so you shouldnt need this cap, thanks

@Ssukriti Should we keep the cap(or a static version) for the transformers package avoid un intended errors like xla_fsdp_v2. We could create github workflow to run tests and then update the cap regularly.

we dont need to keep static version , but yes in optional dependencies PR , @gkumbhat is looking into how to cap and we may cap to next major release. Now that CI/CD with automatically pull new release versions , if we see failing builds, we will update accordingly
the errors we were seeing with xla_fsdp_v2 was actually due to code we wrote , which was good to catch and fix . It was not a API change from transformers, but we were setting env variables incorrectly

In general if there is a specific version that doesn't work, or has a bug ,then we can also ask pip to ignore that particular version.

#54 (comment)

add more unit tests and refactor

Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>

Ssukriti

I think some more tests can be i:

if num_epocs and num_gradient_acc steps is 0 , valueerror is returned
prompt tuning test can test with "prompt_tuning_init": "RANDOM", and TEXT (only Random is tested)

Ssukriti · 2024-03-12T00:20:11Z

+        assert "Simply put, the theory of relativity states that" in output_inference
+
+
+def test_run_train_lora_target_modules():


whats the difference between this and above test? can we combine to 1?

My understanding is that first we check that default target modules are used, then the next one is for custom target modules specified by user and the last for all-linear. I've parameterized it but worth confirming with Anh.

Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>

gkumbhat · 2024-03-12T13:11:34Z

@@ -0,0 +1,22 @@
+# Copyright The IBM Tuning Team


Curious about the copyright notice..Where is this coming from?

IBM Tuning Team was suggested by Raghu, the rest is from caikit

Ssukriti · 2024-03-12T16:34:19Z

+            invalid_params
+        )
+
+        with pytest.raises(ValueError, match=exc_msg):


I generally avoid matching exact error message and just check for valueError with a comment explaining why, but will let this go and I dont think we will update the message much

Ssukriti

Thanks so much!!

* Set up fixtures and data for tests Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com> * Add basic unit tests Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com> * Setting upper bound for transformers Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com> * Ignore aim log files Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com> * Include int num_train_epochs Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com> * Fix formatting Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com> * Add copyright notice Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com> * Address review comments Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com> * Run inference on tuned model Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com> * Trainer downloads model Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com> * add more unit tests and refactor Signed-off-by: Anh-Uong <anh.uong@ibm.com> * Fix formatting Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com> * Add FT unit test and refactor Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com> * Removing transformers upper bound cap Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com> * Address review comments Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com> --------- Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com> Signed-off-by: Anh-Uong <anh.uong@ibm.com> Co-authored-by: Anh-Uong <anh.uong@ibm.com>

tharapalanivel and others added 11 commits March 7, 2024 09:48

Set up fixtures and data for tests

7a7ae5c

Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>

Add basic unit tests

09fb920

Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>

Setting upper bound for transformers

c483ed2

Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>

Ignore aim log files

27f12dc

Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>

Include int num_train_epochs

da7f8cb

Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>

Fix formatting

45a6a05

Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>

Add copyright notice

1158054

Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>

Address review comments

e7dfb06

Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>

Run inference on tuned model

1d9b4ef

Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>

Trainer downloads model

6d912b6

Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>

add more unit tests and refactor

6c87404

Signed-off-by: Anh-Uong <anh.uong@ibm.com>

Ssukriti reviewed Mar 9, 2024

View reviewed changes

tedhtchang mentioned this pull request Mar 11, 2024

Switches dependencies from txt file to toml file #68

Merged

3 tasks

tharapalanivel added 5 commits March 11, 2024 11:45

Merge pull request #1 from tharapalanivel/anh-unit-tests

851d0ec

add more unit tests and refactor

Merge branch 'main' into unit_tests2

e23ab15

Fix formatting

f288cd4

Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>

Add FT unit test and refactor

f958f15

Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>

Removing transformers upper bound cap

3d7b4df

Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>

tharapalanivel marked this pull request as ready for review March 11, 2024 20:41

tharapalanivel requested review from alex-jw-brooks and anhuong as code owners March 11, 2024 20:41

tharapalanivel requested a review from Ssukriti March 11, 2024 20:42

Ssukriti requested changes Mar 12, 2024

View reviewed changes

Address review comments

dc55c72

Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>

gkumbhat reviewed Mar 12, 2024

View reviewed changes

Ssukriti reviewed Mar 12, 2024

View reviewed changes

Ssukriti approved these changes Mar 12, 2024

View reviewed changes

Ssukriti merged commit a716cd7 into foundation-model-stack:main Mar 12, 2024

tharapalanivel deleted the unit_tests2 branch April 9, 2024 20:44

		assert "Simply put, the theory of relativity states that" in output_inference


		def test_run_train_lora_target_modules():

Conversation

tharapalanivel commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of the change

Related issue number

How to verify the PR

Was the PR tested

Uh oh!

Ssukriti commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ssukriti Mar 9, 2024

Choose a reason for hiding this comment

Uh oh!

tedhtchang Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ssukriti Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

gkumbhat Mar 12, 2024

Choose a reason for hiding this comment

Uh oh!

Ssukriti left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Ssukriti Mar 12, 2024

Choose a reason for hiding this comment

Uh oh!

tharapalanivel Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gkumbhat Mar 12, 2024

Choose a reason for hiding this comment

Uh oh!

tharapalanivel Mar 12, 2024

Choose a reason for hiding this comment

Uh oh!

Ssukriti Mar 12, 2024

Choose a reason for hiding this comment

Uh oh!

Ssukriti left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tharapalanivel commented Mar 7, 2024 •

edited

Loading

Ssukriti commented Mar 8, 2024 •

edited

Loading

tedhtchang Mar 11, 2024 •

edited

Loading

Ssukriti left a comment •

edited

Loading

tharapalanivel Mar 12, 2024 •

edited

Loading