KEP-2170: Add unit and E2E tests for model and dataset initializers #2323

seanlaii · 2024-11-09T03:18:51Z

What this PR does / why we need it:
I added unit tests and e2e tests for model and dataset initializers.

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Fixes #2305

Checklist:

Docs included if any changes are user facing

google-oss-prow · 2024-11-09T03:18:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign johnugeorge for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

seanlaii · 2024-11-09T03:19:57Z

pkg/initializer_v2/test/e2e/test_dataset.py

+            # Private HuggingFace dataset test
+            # (
+            #     "HuggingFace - Private dataset",
+            #     "huggingface",
+            #     {
+            #         "storage_uri": "hf://username/private-dataset",
+            #         "use_real_token": True,
+            #         "expected_files": ["config.json", "dataset.safetensors"],
+            #         "expected_error": None
+            #     }
+            # ),
+            # Invalid HuggingFace dataset test


Do we have an access token for testing login and downloading resources from private repo?

Not yet, maybe we can track this in a separate issue that we should create Kubeflow-owned account in HF for the Token.

seanlaii · 2024-11-09T03:21:10Z

pkg/initializer_v2/test/e2e/test_dataset.py

+        current_dir = os.path.dirname(os.path.abspath(__file__))
+        self.temp_dir = tempfile.mkdtemp(dir=current_dir)
+        os.environ[VOLUME_PATH_DATASET] = self.temp_dir


I currently test the dataset/model download by downloading resources to a temp folder and removing the temp folder after the test.

seanlaii · 2024-11-09T03:24:22Z

.github/workflows/test-python.yaml

-        run: pytest ./sdk/python/kubeflow/training/api/training_client_test.py
+        run: |
+          pytest ./sdk/python/kubeflow/training/api/training_client_test.py
+          pytest ./pkg/initializer_v2/test/unit


I currently put the unit test under the training SDK step. Should I add another step?

Yeah, let's add another steps called: Run Python unit tests for v2.

seanlaii · 2024-11-09T03:26:19Z

pkg/initializer_v2/test/conftest.py

+@pytest.fixture
+def real_hf_token():
+    """Fixture to provide real HuggingFace token for E2E tests"""
+    token = os.getenv("HUGGINGFACE_TOKEN")
+    # if not token:
+    #     pytest.skip("HUGGINGFACE_TOKEN environment variable not set")
+    return token


If we have a private token, I will use this fixture to inject the token. If we don't, I can remove this.

coveralls · 2024-11-09T04:04:53Z

Pull Request Test Coverage Report for Build 11758432746

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall first build on initializer-test at 100.0%

Totals
Change from base Build 11758410179:	100.0%
Covered Lines:	77
Relevant Lines:	77

💛 - Coveralls

seanlaii · 2024-11-09T04:14:00Z

.github/workflows/integration-tests.yaml

          python3 -m pip install -e sdk/python; pytest -s sdk/python/test --log-cli-level=debug --namespace=default
        env:
          GANG_SCHEDULER_NAME: ${{ matrix.gang-scheduler-name }}

+      - name: Run specific tests for Python 3.10+


Since match is released in python 3.10, I created another step for the e2e.

Where do you use match in the tests ?

I didn't use match in the tests. match is used in https://github.com/kubeflow/training-operator/blob/master/pkg/initializer_v2/model/__main__.py#L23 and https://github.com/kubeflow/training-operator/blob/master/pkg/initializer_v2/dataset/__main__.py#L23

Oh, good point.
Let's actually use the same Python version that we use in our initializer images: https://github.com/kubeflow/training-operator/blob/master/cmd/initializer_v2/dataset/Dockerfile#L1.
E.g. Python 3.11

seanlaii · 2024-11-09T04:23:24Z

pkg/initializer_v2/test/e2e/test_dataset.py

+                "HuggingFace - Public dataset",
+                "huggingface",
+                {
+                    "storage_uri": "hf://karpathy/tiny_shakespeare",


Does anyone know which dataset/model in huggingface is suitable for the connectivity test?

@seanlaii Which connectivity test do you want to perform ?

I would like to test the actual downloading process and would like to know if there is any recommended dataset/model for testing. I currently choose a dataset that is only 1.11 MB.

Signed-off-by: wei-chenglai <[email protected]>

seanlaii · 2024-11-26T01:39:54Z

Hi @andreyvelich ,

Could you help review this PR? I have some questions. Once the SDK's PR gets approved, I will modify it accordingly.

Thank you!

andreyvelich · 2024-11-26T14:50:27Z

@seanlaii Sorry for the delay, sure, I will review it today

andreyvelich · 2024-11-26T15:37:32Z

pkg/initializer_v2/test/conftest.py

@@ -0,0 +1,52 @@
+import os


I would suggest we put the e2e tests under /test/e2e/initializer_v2/.... and the unit tests close to the actual files, e.g. /pkg/initializer_v2/dataset/huggingface_test.py.
That is what we do for Go, also we've done the same for SDK V1 unit tests: https://github.com/kubeflow/training-operator/tree/c6e0a832afd019a7d1fa8fa9442b81caf53b54c0/sdk/python/kubeflow/training/api

WDYT @seanlaii @kubeflow/wg-training-leads @Electronic-Waste @droctothorpe ?

@andreyvelich I agree with you since it would be more clear.

Sounds good. Thanks for the advice! I will change it.

andreyvelich

Thank you for this effort @seanlaii!
I left my initial thoughts.
Please take a look @Electronic-Waste @deepanker13 @kubeflow/wg-training-leads @varshaprasad96 @akshaychitneni @saileshd1402

andreyvelich · 2024-11-26T15:40:23Z

.github/workflows/integration-tests.yaml

          python3 -m pip install -e sdk/python; pytest -s sdk/python/test --log-cli-level=debug --namespace=default
        env:
          GANG_SCHEDULER_NAME: ${{ matrix.gang-scheduler-name }}

+      - name: Run specific tests for Python 3.10+


Where do you use match in the tests ?

andreyvelich · 2024-11-26T15:41:06Z

.github/workflows/test-python.yaml

-        run: pytest ./sdk/python/kubeflow/training/api/training_client_test.py
+        run: |
+          pytest ./sdk/python/kubeflow/training/api/training_client_test.py
+          pytest ./pkg/initializer_v2/test/unit


Yeah, let's add another steps called: Run Python unit tests for v2.

andreyvelich · 2024-11-26T15:44:51Z

pkg/initializer_v2/test/unit/dataset/test_dataset.py

@@ -0,0 +1,86 @@
+import runpy


Why do we want to use runpy to execute the tests ?

I use runpy to run the dataset, and model modules.

Do you really need to execute __main__.py as part of your unit tests ?
E.g. the entire logic can be tested in the dataset/huggingface_test.py and model/huggingface_test.py

I guess, you can verify that __main__.py executes correctly as part of your E2E tests.
WDYT @seanlaii ?

Yes, I can verify the __main__.py in the E2E tests.
Or perhaps we can wrap the logic in the script to a function, e.g., main(), so we can avoid using runpy to run the script, and just validate the main() function in huggingface_test.py.
The reason I try to execute the script is mainly to validate the overall flow and some exceptions.

I see, that makes sense. Maybe we should wrap our logic under main() func in the __main__.py file given that usually runpy is used for integration tests, not for unit testing.
So we can have this. __main__.py:

def main(): # logic here. if __name__ == "__main__": main()

main_test.py: Contains unit tests for the main() function.

WDYT @seanlaii ?

Yes, it sounds good to me.

andreyvelich · 2024-11-26T15:49:47Z

pkg/initializer_v2/test/unit/model/test_model_config.py

+from pkg.initializer_v2.model.config import HuggingFaceModelInputConfig
+
+
+def test_huggingface_model_config_creation():


Should we use @pytest.mark.parametrize with tests cases here for consistency across all unit tests ?

andreyvelich · 2024-11-26T15:52:00Z

pkg/initializer_v2/test/unit/model/test_model.py

@@ -0,0 +1,86 @@
+import runpy


I would name this file huggingface_test.py where we are going to unit tests all functionality from the huggingface.py file.

andreyvelich · 2024-11-26T15:52:38Z

pkg/initializer_v2/test/unit/test_utils.py

@@ -0,0 +1,25 @@
+import pytest


You can name this as utils_test.py

andreyvelich · 2024-11-27T23:32:33Z

pkg/initializer_v2/test/e2e/test_model.py

+from sdk.python.kubeflow.storage_initializer.constants import VOLUME_PATH_MODEL
+
+
+class TestModelE2E:


@seanlaii @kubeflow/wg-training-leads @deepanker13 @Electronic-Waste @saileshd1402 What do you think about actually using Kubernetes to perform E2E tests for our initializers ?
E.g. we can deploy a single Pod that runs two initContainer for initializers and one Container to just verify that model and dataset exists under /workspace/model and /workspace/dataset dirs.

In that case, in our E2Es we verify that our Docker containers actually work to initialize assets.

Do we see any values in tests that I propose compare to running just initializers Python scripts ?

andreyvelich · 2024-11-27T23:38:58Z

pkg/initializer_v2/test/unit/dataset/test_dataset.py

@@ -0,0 +1,86 @@
+import runpy


Do you really need to execute __main__.py as part of your unit tests ?
E.g. the entire logic can be tested in the dataset/huggingface_test.py and model/huggingface_test.py

I guess, you can verify that __main__.py executes correctly as part of your E2E tests.
WDYT @seanlaii ?

google-oss-prow bot requested review from jinchihe and kuizhiqing November 9, 2024 03:18

google-oss-prow bot added the size/XL label Nov 9, 2024

seanlaii commented Nov 9, 2024

View reviewed changes

seanlaii force-pushed the initializer-test branch from f4167e5 to f6345df Compare November 9, 2024 04:00

seanlaii force-pushed the initializer-test branch from f6345df to 1887c5b Compare November 9, 2024 04:11

seanlaii commented Nov 9, 2024

View reviewed changes

seanlaii force-pushed the initializer-test branch 3 times, most recently from bd1c8fd to 8930b80 Compare November 9, 2024 05:24

KEP-2170: Add unit and E2E tests for model and dataset initializers

c6e0a83

Signed-off-by: wei-chenglai <[email protected]>

seanlaii force-pushed the initializer-test branch from 8930b80 to c6e0a83 Compare November 9, 2024 18:17

andreyvelich reviewed Nov 26, 2024

View reviewed changes

andreyvelich reviewed Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-2170: Add unit and E2E tests for model and dataset initializers #2323

KEP-2170: Add unit and E2E tests for model and dataset initializers #2323

seanlaii commented Nov 9, 2024

google-oss-prow bot commented Nov 9, 2024

seanlaii Nov 9, 2024

andreyvelich Nov 26, 2024

seanlaii Nov 9, 2024 •

edited

Loading

seanlaii Nov 9, 2024

andreyvelich Nov 26, 2024

seanlaii Nov 9, 2024

coveralls commented Nov 9, 2024 •

edited

Loading

seanlaii Nov 9, 2024

andreyvelich Nov 26, 2024

seanlaii Nov 27, 2024 •

edited

Loading

andreyvelich Nov 27, 2024

seanlaii Nov 9, 2024

andreyvelich Nov 27, 2024

seanlaii Nov 28, 2024

seanlaii commented Nov 26, 2024 •

edited

Loading

andreyvelich commented Nov 26, 2024

andreyvelich Nov 26, 2024

Electronic-Waste Nov 26, 2024

seanlaii Nov 27, 2024

andreyvelich left a comment

andreyvelich Nov 26, 2024

andreyvelich Nov 26, 2024

andreyvelich Nov 26, 2024

seanlaii Nov 27, 2024 •

edited

Loading

andreyvelich Nov 27, 2024

seanlaii Nov 28, 2024

andreyvelich Nov 29, 2024

seanlaii Nov 30, 2024

andreyvelich Nov 26, 2024

andreyvelich Nov 26, 2024

andreyvelich Nov 26, 2024

andreyvelich Nov 27, 2024 •

edited

Loading

andreyvelich Nov 27, 2024

		from pkg.initializer_v2.model.config import HuggingFaceModelInputConfig


		def test_huggingface_model_config_creation():

		from sdk.python.kubeflow.storage_initializer.constants import VOLUME_PATH_MODEL


		class TestModelE2E:

KEP-2170: Add unit and E2E tests for model and dataset initializers #2323

Are you sure you want to change the base?

KEP-2170: Add unit and E2E tests for model and dataset initializers #2323

Conversation

seanlaii commented Nov 9, 2024

google-oss-prow bot commented Nov 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlaii Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 9, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11758432746

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlaii Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlaii commented Nov 26, 2024 • edited Loading

andreyvelich commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlaii Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlaii Nov 9, 2024 •

edited

Loading

coveralls commented Nov 9, 2024 •

edited

Loading

seanlaii Nov 27, 2024 •

edited

Loading

seanlaii commented Nov 26, 2024 •

edited

Loading

seanlaii Nov 27, 2024 •

edited

Loading

andreyvelich Nov 27, 2024 •

edited

Loading