Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Map multiple vineyard objects to one volume while specifying the common prefix in the kubeflow benchmark #1578

Merged
merged 2 commits into from
Sep 25, 2023

Conversation

dashanji
Copy link
Member

What do these changes do?

  • Map multiple vineyard objects to one volume while specifying the common prefix.
  • Update the kubeflow benchmark result and the related tutorial.

Related issue number

Fixes #1568

…mmon prefix.

* Update the kubeflow benchmark result and the related tutorial.

Signed-off-by: Ye Cao <[email protected]>
@dashanji dashanji requested a review from sighingnow September 22, 2023 05:29
@@ -14,6 +14,7 @@ Prerequisites
guide `Initialize Kubernetes Cluster`_ to create one.
- Install the `Vineyardctl`_ by following the official guide.
- Install the `Argo Workflow CLI`_ by following the official guide.
- Install the kfp package with version < 2.0.0.
Copy link
Member

Choose a reason for hiding this comment

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

Why we requires <2.0.0 here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The current demo is based on the kfp v1.8.0. In kfp v2, we need to rewrite the demo with the new API.

Copy link
Member Author

Choose a reason for hiding this comment

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

As the kfp v2 has some breaking changes compared to v1. Backgound: https://www.kubeflow.org/docs/components/pipelines/v2/migration/

Actually, I think it doesn't matter as they are complied as the argo workflow YAML finally.

+------------+------------------+---------------+
| 15000 Mi | 332s | 286s |
| 15000 Mi | 252s | 298s |
+------------+------------------+---------------+
Copy link
Member

Choose a reason for hiding this comment

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

Why there are so huge improvements on "without vineyard"? and "with vineyard" becomes slower than "without vineyard"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, wrong writing here.

+------------+------------------+---------------+
| 15000 Mi | 128.6s | 0.04s |
| 15000 Mi | 61.7s | 0.04s |
+------------+------------------+---------------+
Copy link
Member

Choose a reason for hiding this comment

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

Why this PR affects the performances of "without vineyard" case?

Copy link
Member Author

Choose a reason for hiding this comment

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

The previous time is wrong as it is mixed with the initial reading time. My bad.

@@ -1,12 +1,10 @@
FROM python:3.10

RUN pip3 install --no-cache-dir pandas requests scikit-learn numpy vineyard
RUN pip3 install pandas requests scikit-learn numpy vineyard
Copy link
Member

Choose a reason for hiding this comment

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

Use --no-cache-dir to let pip cleanup the downloaded zips/wheels after installation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Signed-off-by: Ye Cao <[email protected]>
@dashanji dashanji requested a review from sighingnow September 22, 2023 09:43
@sighingnow sighingnow merged commit 4e78039 into v6d-io:main Sep 25, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supports mapping multiple vineyard objects to the same volume in the vineyard csi driver
2 participants