feat(cache): KEP-2655: Support provisioning of cache with Kubeflow SDK #112
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Pull Request Test Coverage Report for Build 18136229986Details
💛 - Coveralls |
| metadata_loc: str | ||
| schema_name: str | ||
| table_name: str | ||
| env: Optional[dict[str, str]] = None |
There was a problem hiding this comment.
Do we want to be explicit which optional values user can set rather than giving them arbitrary env configuration in DataCacheInitializer?
kubeflow/trainer/utils/utils.py
Outdated
| if not isinstance(dataset, types.HuggingFaceDatasetInitializer): | ||
| return None | ||
| if isinstance(dataset, types.HuggingFaceDatasetInitializer): | ||
| # TODO (andreyvelich): Support more parameters. |
There was a problem hiding this comment.
you probably can remove this TODO for now.
| # TODO (andreyvelich): Support more parameters. |
| ), | ||
| ) | ||
| return dataset_initializer | ||
| elif isinstance(dataset, types.DataCacheInitializer): |
There was a problem hiding this comment.
you probably can make this more generic, since ENV names are equal to the DataClass field names:
envs = []
for f in fields(dataset):
name = f.name.upper()
value = getattr(dataset, f.name)
envs.append(models.IoK8sApiCoreV1EnvVar(name=name, value=str(value)))
kubeflow/trainer/types/types.py
Outdated
| cluster_size: int | ||
| metadata_loc: str | ||
| schema_name: str | ||
| table_name: str |
There was a problem hiding this comment.
Would it be possible to leverage storage_uri to set location or it has some limitations ?
e.g.
cache://<CATALOG_NAME>/<DATABASE_NAME>/<TABLE_NAME>
I can imagine that metadata location can be set separately.
There was a problem hiding this comment.
As discussed, lets have uri to cache://<DATABASE_NAME>/<TABLE_NAME> and use metadata_loc
|
/milestone v0.2 |
Signed-off-by: Akshay Chitneni <[email protected]>
What this PR does / why we need it:
Adds cache initializer in sdk
Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Fixes # kubeflow/trainer#2866
Checklist: