KEP-2170: Create model and dataset initializers #2303

andreyvelich · 2024-10-23T21:45:06Z

I created model and dataset initializers.
Initially, we will only support HF for the demo purposes.
I will create dedicated issue to support more providers.

/assign @kubeflow/wg-training-leads @varshaprasad96 @akshaychitneni @deepanker13 @helenxie-bit @Electronic-Waste @saileshd1402 @kannon92

Signed-off-by: Andrey Velichkevich <[email protected]>

coveralls · 2024-10-23T21:49:51Z

Pull Request Test Coverage Report for Build 11517758882

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 11507477280:	0.0%
Covered Lines:	77
Relevant Lines:	77

💛 - Coveralls

kannon92 · 2024-10-23T22:36:23Z

Should we consider unit or e2e tests for this?

.gitignore

andreyvelich · 2024-10-23T22:39:22Z

Should we consider unit or e2e tests for this?

Yeah, I will open dedicated issue for it.

Co-authored-by: Kevin Hannon <[email protected]> Signed-off-by: Andrey Velichkevich <[email protected]>

kannon92 · 2024-10-23T22:54:01Z

pkg/initiailizer_v2/utils/utils.py

+
+# Get DataClass config from the environment variables.
+# Env names must be equal to the DataClass parameters.
+def get_config_from_env(config) -> Dict[str, str]:


do you want some typing hints for config

Actually, I wasn't able to find Python type that can represent DataClass, as described here: https://stackoverflow.com/questions/54668000/type-hint-for-an-instance-of-a-non-specific-dataclass#:~:text=Despite%20its%20name%2C%20dataclasses.dataclass%20doesn%27t%20expose%20a%20class%20interface..
Any ideas on how we can add the type hint for it @kannon92 ?

Ah I see. Then it’s fine to go without.

I’m not much of a Python dev these days so I don’t know about the typing for data classes.

shall we use like this
config : Union[DataClassA, DataClassB]

I tried this, it won't work. E.g. I can see this error from PyLance:

Argument of type "type[HuggingFaceDatasetConfig]" cannot be assigned to parameter "config" of type "HuggingFaceDatasetConfig" in function "get_config_from_env" "type[type]" is not assignable to "type[HuggingFaceDatasetConfig]"

from dataclasses import fields from typing import Dict import os @dataclass class HuggingFaceModelInputConfig: storage_uri: str access_token: Optional[str] = None def get_config_from_env(config: Union[HuggingFaceModelInputConfig]) -> Dict[str, str]: config_from_env = {} for field in fields(config): config_from_env[field.name] = os.getenv(field.name.upper()) return config_from_env cf1 = get_config_from_env(HuggingFaceModelInputConfig) print(cf1)

This is working @andreyvelich

@deepanker13 What type validator do you use in your IDE ?
My VSCode complains with the error that I added above.
I am using PyLance VSCode extension: https://github.com/microsoft/pylance-release

saileshd1402 · 2024-10-24T05:46:13Z

pkg/initiailizer_v2/model/huggingface.py

+            repo_id=model_uri,
+            local_dir=constants.VOLUME_PATH_MODEL,
+            allow_patterns=["*.json", "*.safetensors", "*.model"],
+            ignore_patterns=["*.msgpack", "*.h5", "*.bin"],


we can consider ignoring ".pt" and ".pth" files as well

@saileshd1402 Do we know what files have .pt and .pth extension ?

They store model weights similar to .safetensors and .bin but are outdated now. For example: In meta-llama/Llama-3.1-8B there is a folder called "/original" where consolidated.00.pth stores same data as ".safetensors" but is outdated. But would like to know if others think those files are important

@lizzzcai do you have any thoughts on whether we can exclude .pth and .pt files from model download ?
I noticed that you suggested to add the same ignore_patterns to KServe storage initializer: kserve/kserve#3584 (comment)

Hi @andreyvelich, I don't see an issue to exclude .pth, .pt and .bin, as safetensors should be a preferred format and it has better security compared to others. Assuming that the model being downloaded provides safetensors format (most of the popular model should have).

If you want to support multiple formats, can check how vLLM support it.

I see, thanks for the info!
I think, in the future we should follow the vLLM approach.

deepanker13 · 2024-10-24T18:11:07Z

.github/workflows/publish-core-images.yaml

@@ -30,6 +30,14 @@ jobs:
            dockerfile: cmd/training-operator.v2alpha1/Dockerfile
            platforms: linux/amd64,linux/arm64,linux/ppc64le
            tag-prefix: v2alpha1
+          - component-name: model-initiailizer-v2


There is a typo at multiple places

initializer

Great catch! Let me fix that.

deepanker13 · 2024-10-24T18:30:22Z

pkg/initiailizer_v2/dataset/huggingface.py

+        huggingface_hub.snapshot_download(
+            repo_id=dataset_uri,
+            repo_type="dataset",
+            local_dir=constants.VOLUME_PATH_DATASET,


To speed up things should we set max_workers equal to number of files getting downloaded, currently it downloads 8 files parallely?

Do we have any benchmarks that show that setting of max_workers to number of files speedup download time ?
What if we don't have enough CPUs for all concurrent threads ?

deepanker13 · 2024-10-24T18:35:59Z

cmd/initiailizer_v2/model/Dockerfile

+WORKDIR /workspace
+
+# Copy the required Python modules.
+COPY cmd/initiailizer_v2/model/requirements.txt .


why is the folder named as cmd? It is containing docker files and requirements.txt

We want to be consistent across all Training Operator components, like we do in Katib: https://github.com/kubeflow/katib/tree/master/cmd
E.g.
cmd - contains binaries/dockerfiles for execution.
pkg - contains the actual backend.

varshaprasad96 · 2024-10-24T20:23:53Z

pkg/initiailizer_v2/dataset/huggingface.py

+        logging.info(f"Config for HuggingFace dataset initiailizer: {config_dict}")
+        self.config = HuggingFaceDatasetConfig(**config_dict)
+
+    def download_dataset(self):


Do we have unit tests for these, or would it be taken care in e2e? Downloading models from HF was having issues previously, it would be helpful to have it tested.

Nvmd, just realised we have: #2305

Signed-off-by: Andrey Velichkevich <[email protected]>

lizzzcai · 2024-10-25T02:28:56Z

pkg/initiailizer_v2/model/huggingface.py

+        huggingface_hub.snapshot_download(
+            repo_id=model_uri,
+            local_dir=constants.VOLUME_PATH_MODEL,
+            allow_patterns=["*.json", "*.safetensors", "*.model"],


For "*.safetensors" and "*.model" in the allow_patterns, I would say it works in most of the cases.
However, for model like mistralai/Mistral-7B-Instruct-v0.3, it has consolidated.safetensors and tokenizer.model.v3 (so called v3 format in Mistral, check their download example here).

snapshot_download(repo_id="mistralai/Mistral-7B-Instruct-v0.3", allow_patterns=["params.json", "consolidated.safetensors", "tokenizer.model.v3"], local_dir=mistral_models_path)

In this case, downloading the above mistral model with the current allow_patterns, the downloaded size will be 29 GB (double from the actual size 14.5 GB). Probably you need some logic to handle the Mistral model.

Thanks for sharing! Let me add it to the TODO list.

Electronic-Waste

I wonder if we can add some comment somewhere to let users know we only support downloading models/datasets from HuggingFace now?

It might be more user-friendly :)

andreyvelich · 2024-10-25T11:58:31Z

I wonder if we can add some comment somewhere to let users know we only support downloading models/datasets from HuggingFace now?

Yes, we are planning to add supported dataset and model providers to the website.
Additionally, I will create tracking issue to support more providers (S3, GCS, etc.)

Signed-off-by: Andrey Velichkevich <[email protected]>

andreyvelich · 2024-10-25T22:18:15Z

Are there any other comments before we can move forward with this initial PR ?
/assign @kannon92 @Electronic-Waste @deepanker13 @varshaprasad96 @saileshd1402

google-oss-prow · 2024-10-25T22:18:19Z

@andreyvelich: GitHub didn't allow me to assign the following users: varshaprasad96, saileshd1402.

Note that only kubeflow members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

Are there any other comments before we can move forward with this initial PR ?
/assign @kannon92 @Electronic-Waste @deepanker13 @varshaprasad96 @saileshd1402

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tenzen-y

Let's address other issues in a follow up.
/lgtm
/approve

tenzen-y · 2024-10-27T18:01:02Z

cmd/initializer_v2/dataset/Dockerfile

@@ -0,0 +1,13 @@
+FROM python:3.11-alpine


Suggested change

FROM python:3.11-alpine

FROM python:3.11-slim-bookworm

@andreyvelich Could you use the Debian image since the Alpine has a performance penalty due to with musl libc?
Python still depends on the C codes.

Sure, let me create an issue

google-oss-prow · 2024-10-27T18:01:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [tenzen-y]

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

andreyvelich added 3 commits October 23, 2024 22:15

KEP-2170: Create model and dataset initializers

fe481b7

Signed-off-by: Andrey Velichkevich <[email protected]>

Add abstract classes

59d3224

Signed-off-by: Andrey Velichkevich <[email protected]>

Add storage URI to config

45c860c

Signed-off-by: Andrey Velichkevich <[email protected]>

google-oss-prow bot requested review from jinchihe and kuizhiqing October 23, 2024 21:45

google-oss-prow bot added the size/L label Oct 23, 2024

kannon92 reviewed Oct 23, 2024

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

Update .gitignore

468500d

Co-authored-by: Kevin Hannon <[email protected]> Signed-off-by: Andrey Velichkevich <[email protected]>

andreyvelich mentioned this pull request Oct 23, 2024

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

Open

kannon92 reviewed Oct 23, 2024

View reviewed changes

saileshd1402 reviewed Oct 24, 2024

View reviewed changes

deepanker13 reviewed Oct 24, 2024

View reviewed changes

varshaprasad96 reviewed Oct 24, 2024

View reviewed changes

Fix the misspelling for initializer

5c39812

Signed-off-by: Andrey Velichkevich <[email protected]>

andreyvelich force-pushed the issue-2210-storage-initializer branch from fa77453 to 5c39812 Compare October 25, 2024 02:03

lizzzcai reviewed Oct 25, 2024

View reviewed changes

Electronic-Waste reviewed Oct 25, 2024

View reviewed changes

Add .pt and .pth to ignore_patterns

2e8a518

Signed-off-by: Andrey Velichkevich <[email protected]>

andreyvelich force-pushed the issue-2210-storage-initializer branch from 2cf2c5e to 2e8a518 Compare October 25, 2024 11:58

google-oss-prow bot assigned deepanker13, Electronic-Waste and kannon92 Oct 25, 2024

tenzen-y reviewed Oct 27, 2024

View reviewed changes

google-oss-prow bot assigned tenzen-y Oct 27, 2024

google-oss-prow bot added the lgtm label Oct 27, 2024

google-oss-prow bot added the approved label Oct 27, 2024

google-oss-prow bot merged commit 3f7ec16 into kubeflow:master Oct 27, 2024
43 checks passed

andreyvelich mentioned this pull request Oct 28, 2024

Use Debian images for Python components in the Training Operator V2 #2311

Open

andreyvelich deleted the issue-2210-storage-initializer branch October 28, 2024 01:43

This was referenced Oct 28, 2024

Update Dockerfile with python debian image mani1soni/training-operator#1

Merged

Update Dockerfile with python debian image in cmd/initializer_v2/dataset/Dockerfile #2312

Open

KEP-2170: Create model and dataset initializers #2303

KEP-2170: Create model and dataset initializers #2303

Conversation

andreyvelich commented Oct 23, 2024 • edited Loading

coveralls commented Oct 23, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11517758882

Details

💛 - Coveralls

kannon92 commented Oct 23, 2024

andreyvelich commented Oct 23, 2024

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

deepanker13 Oct 25, 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

saileshd1402 Oct 24, 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

deepanker13 Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepanker13 Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

andreyvelich Oct 25, 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

Electronic-Waste left a comment

Choose a reason for hiding this comment

andreyvelich commented Oct 25, 2024

andreyvelich commented Oct 25, 2024

google-oss-prow bot commented Oct 25, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

tenzen-y Oct 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-prow bot commented Oct 27, 2024

andreyvelich commented Oct 23, 2024 •

edited

Loading

coveralls commented Oct 23, 2024 •

edited

Loading

deepanker13 Oct 25, 2024 •

edited

Loading

saileshd1402 Oct 24, 2024 •

edited

Loading

deepanker13 Oct 24, 2024 •

edited

Loading

deepanker13 Oct 24, 2024 •

edited

Loading

andreyvelich Oct 25, 2024 •

edited

Loading

tenzen-y Oct 27, 2024 •

edited

Loading