Skip to content
This repository was archived by the owner on Sep 10, 2025. It is now read-only.

Conversation

@pmabbo13
Copy link
Contributor

@pmabbo13 pmabbo13 commented Jul 19, 2022

Description

Add integration tests for both the T5 model (encoder-only and encoder-decoder)

Process

  • Upload expected output for example input sequence, sourced from HuggingFace after passing in same input to corresponding HF model -> test/asset/t5.base.encoder.output.pt, test/asset/t5.base.output
  • Add integration tests to verify that T5_BASE_ENCODER and T5_BASE outputs match the references from HF
  • Test that bundler methods ( build_model(), get_model(), config() ) are working as expected, and returning the correct error messages when invalid input args are provided.

Testing

pytest test/prototype/integration_tests/test_models.py
pytest test/prototype/models/test_models.py

Follow-Up

  • Add testing for model training after LM head has been added (need LM head otherwise cannot compute cross-entropy loss) Prepare T5 Model for Language Generation #1862
  • Investigate if model is torch-scriptable. This is unclear at the moment because the pytorch transformer implementation indicates that transformers are not torch-scriptable if normalization happens first (which it does for T5), and when the query, key, and values are not equal (which is the case for transformer and t5 decoders). Make T5 model torchscriptable #1876

@pmabbo13 pmabbo13 marked this pull request as ready for review July 19, 2022 21:08
Copy link
Contributor

@Nayef211 Nayef211 left a comment

Choose a reason for hiding this comment

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

LGTM! When we decide to move the model and the tests to the main folder, we should look into parameterizing the model and integration tests since all of the tests are almost exactly the same as the ones we have here and here.


def test_t5_base_encoder_model(self):
expected_asset_name = "t5.base.encoder.output.pt"
model_input = torch.tensor([[1, 2, 3, 4, 5, 6], [7, 8, 9, 0, 0, 0]])
Copy link
Contributor

Choose a reason for hiding this comment

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

Just something to note, when we implement the transform for the model, we probably want to update the test to pass in an input string to the _t5_model method. The helper function will be responsible for applying the transform on the input string to get the tensor that can be passed into the model (code pointer). The T5Bundle class will also need to be updated to store the model transform as a member variable (code pointer).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, will keep this in mind for the next task!

Copy link
Contributor

@parmeet parmeet left a comment

Choose a reason for hiding this comment

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

Overall LGTM! Thanks for adding the integration test.

@pmabbo13 pmabbo13 merged commit ed69973 into pytorch:main Jul 21, 2022
@pmabbo13 pmabbo13 deleted the feature/t5-integration-tests branch July 21, 2022 15:39
pmabbo13 added a commit to pmabbo13/text that referenced this pull request Jul 21, 2022
* test bundler api

* upload reference results to verify model correctness

* test for model correctness against reference results

* Revert "test for model correctness against reference results"

This reverts commit 9837f4a.

* Revert "upload reference results to verify model correctness"

This reverts commit bea35bc.

* Revert "test bundler api"

This reverts commit d8fe63e.

* test bundler api

* test bundler api

* upload reference results to verify model correctness

* test for model correctness against reference results

* nit correction

* test bundler when model is encoder-only

* correcting typo

* remove redundant test for encoder-only
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants