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

[python sdk] add v1beta1 models #1252

Merged
merged 10 commits into from
Aug 18, 2020

Conversation

sperlingxx
Copy link
Member

@sperlingxx sperlingxx commented Jul 3, 2020

  1. Add replace v1alpha3 python models with v1beta1 python models.
  2. Provide scripts to update python SDK.
  3. Change default katib version in python SDK from v1alpha3 to v1beta1.
  4. Upgrade python SDK version to 0.0.3.

What's your opinion about this PR, @prem0912 @andreyvelich @gaocegege @johnugeorge ?

@kubeflow-bot
Copy link

This change is Reviewable

Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

The idea LGTM. But I am not the Python expert.

/cc @jinchihe Would you mind helping review it?

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

@sperlingxx Thank you for doing this!

Not sure what is the best way to support both version (v1alpha3 and v1beta1).
Store everything under /models?
Or we can have /sdk/v1alpha3 and /sdk/v1beta1.
What do you think @jinchihe @gaocegege @johnugeorge @prem0912 ?

@sperlingxx can you also add katib_client for v1beta1 ?

And maybe add one more example for the new version.

sdk/python/docs/V1beta1TrialTemplate.md Outdated Show resolved Hide resolved
@gaocegege
Copy link
Member

Or we can have /sdk/v1alpha3 and /sdk/v1beta1.

I think it works.

@sperlingxx
Copy link
Member Author

sperlingxx commented Jul 7, 2020

@sperlingxx Thank you for doing this!

Not sure what is the best way to support both version (v1alpha3 and v1beta1).
Store everything under /models?
Or we can have /sdk/v1alpha3 and /sdk/v1beta1.
What do you think @jinchihe @gaocegege @johnugeorge @prem0912 ?

@sperlingxx can you also add katib_client for v1beta1 ?

And maybe add one more example for the new version.

Hi @andreyvelich , after second thoughts, I found it is not necessary to support out-dated versions, which will only make manitain work really complicated. So, I removed v1Alpha3 stuff. And add scripts to simplify the updation of python SDK.
Examples are still not updated to v1beta1, since I doesn't have proper environment to run the notebook files for now.

@jinchihe
Copy link
Member

jinchihe commented Jul 7, 2020

@gaocegege @sperlingxx That's huge, I will take a look as soon as possible. Great contribution!

Copy link
Member

@jinchihe jinchihe left a comment

Choose a reason for hiding this comment

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

Looks great.
@sperlingxx Would you mind to add E2E test cases (or Notebook example) for the Python SDK to ensure the basic function works fine? and some unit test cases have been generated automatically, better to add those cases to CI tests. Thanks.

@@ -1,11 +1,11 @@
# V1alpha3ExperimentStatus
# V1beta1ExperimentStatus

## Properties
Name | Type | Description | Notes
------------ | ------------- | ------------- | -------------
**completion_time** | [**V1Time**](V1Time.md) | Represents time when the Experiment was completed. It is not guaranteed to be set in happens-before order across separate operations. It is represented in RFC3339 form and is in UTC. | [optional]
Copy link
Member

Choose a reason for hiding this comment

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

Seems V1Time.md missed? or is not generated. I guess that should be have traceback if this is not generated.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jinchihe Yes, V1Time is not generated. Because it belongs to kubernetes/apimachinery rather than katib. V1UnstructuredUnstructured has the same problem. I can move their definitions from md files, but maybe the better way is adding them manually, just like tf-operator ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've applied the second approach.

Copy link
Member

Choose a reason for hiding this comment

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

I remember we need add related flag and then generate that. But I think that's OK to add that manually, that should be work.

sdk/python/docs/V1beta1ExperimentStatus.md Outdated Show resolved Hide resolved
@andreyvelich
Copy link
Member

@sperlingxx Thank you for doing this!
Not sure what is the best way to support both version (v1alpha3 and v1beta1).
Store everything under /models?
Or we can have /sdk/v1alpha3 and /sdk/v1beta1.
What do you think @jinchihe @gaocegege @johnugeorge @prem0912 ?
@sperlingxx can you also add katib_client for v1beta1 ?
And maybe add one more example for the new version.

Hi @andreyvelich , after second thoughts, I found it is not necessary to support out-dated versions, which will only make manitain work really complicated. So, I removed v1Alpha3 stuff. And add scripts to simplify the updation of python SDK.
Examples are still not updated to v1beta1, since I doesn't have proper environment to run the notebook files for now.

Why do you want to delete v1alpha3 version of SDK? Can we support 2 versions: sdk/v1alpha3 and sdk/v1beta1?
Currently, Kubeflow 1.1 uses Katib v1alpha3 release and on the website we say, that users can use Katib SDK: https://master.kubeflow.org/docs/components/hyperparameter-tuning/overview/#katib-interfaces.

@sperlingxx
Copy link
Member Author

@sperlingxx Thank you for doing this!
Not sure what is the best way to support both version (v1alpha3 and v1beta1).
Store everything under /models?
Or we can have /sdk/v1alpha3 and /sdk/v1beta1.
What do you think @jinchihe @gaocegege @johnugeorge @prem0912 ?
@sperlingxx can you also add katib_client for v1beta1 ?
And maybe add one more example for the new version.

Hi @andreyvelich , after second thoughts, I found it is not necessary to support out-dated versions, which will only make manitain work really complicated. So, I removed v1Alpha3 stuff. And add scripts to simplify the updation of python SDK.
Examples are still not updated to v1beta1, since I doesn't have proper environment to run the notebook files for now.

Why do you want to delete v1alpha3 version of SDK? Can we support 2 versions: sdk/v1alpha3 and sdk/v1beta1?
Currently, Kubeflow 1.1 uses Katib v1alpha3 release and on the website we say, that users can use Katib SDK: https://master.kubeflow.org/docs/components/hyperparameter-tuning/overview/#katib-interfaces.

Okay, I'll add v1alpha3 back.

@andreyvelich
Copy link
Member

Okay, I'll add v1alpha3 back.

Thanks @sperlingxx.

What do you think about folder structure proposed here: #1252 (review) with sdk/v1alpha3 and sdk/v1beta1, so we can easily manage 2 version in the repo?

Also, is it possible to handle 2 versions in pip and what do we need for that?
For example, if user wants to install v1alpha3, run: pip install kubeflow-katib=v1alpha3, for v1beta1 run: pip install kubeflow-katib=v1beta1.
I am not sure what is the best way to handle both version of API in python SDK.
What do you think @jinchihe ?

@prem0912
Copy link
Contributor

prem0912 commented Jul 9, 2020

@andreyvelich @sperlingxx It will be better to have with version like pip install kubeflow-katib=0.0.2-v1alpha3, for v1beta1 run: pip install kubeflow-katib=0..0.3-v1beta1.

@sperlingxx
Copy link
Member Author

Okay, I'll add v1alpha3 back.

Thanks @sperlingxx.

What do you think about folder structure proposed here: #1252 (review) with sdk/v1alpha3 and sdk/v1beta1, so we can easily manage 2 version in the repo?

Also, is it possible to handle 2 versions in pip and what do we need for that?
For example, if user wants to install v1alpha3, run: pip install kubeflow-katib=v1alpha3, for v1beta1 run: pip install kubeflow-katib=v1beta1.
I am not sure what is the best way to handle both version of API in python SDK.
What do you think @jinchihe ?

Since users of KatibClient can choose katib api versions by setting environment variableEXPERIMENT_VERSON, to me, it looks fine to provide both apiVersions in python SDK. In terms of python sdk version, I think it can be independent to katib apiVersion. For instance, I update version of katib python SDK from 0.0.2 to 0.0.3 in this PR. In future, we will increase the version number when meets large modification.

@andreyvelich
Copy link
Member

SGTM @sperlingxx.
I agree with @prem0912 about version: 0.0.2-v1alpha3 for v1alpha3 and 0.0.3-v1beta1 for v1beta1.

@sperlingxx
Copy link
Member Author

Any updates?

@andreyvelich
Copy link
Member

@sperlingxx Do you have plans do create /sdk/python/v1alpha3 and /sdk/python/v1beta1 for split versions?

@sperlingxx
Copy link
Member Author

@sperlingxx Do you have plans do create /sdk/python/v1alpha3 and /sdk/python/v1beta1 for split versions?

I have created separate folders for each version.

@sperlingxx
Copy link
Member Author

/retest

@andreyvelich
Copy link
Member

@sperlingxx You need to rebase to pass the tests.

@sperlingxx
Copy link
Member Author

@andreyvelich Done.

### pip install

```sh
pip install kubeflow-katib
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to add version here for v1beta1 and v1alpha3?

Copy link
Member Author

Choose a reason for hiding this comment

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

@andreyvelich
I've changed pip command of v1alpha3 README.md to pip install kubeflow-katib==0.0.2. To make sure v1alpha3 won't acquire v1beta1 package which is latest (default) version.
And I think we also need to upload 0.0.3 version onto https://pypi.org/project/kubeflow-katib/#files after this PR merged. I believe I don't have the authority to do this.

Copy link
Member

Choose a reason for hiding this comment

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

@prem0912 Can you give us permissions to upload versions in this project, please?

/cc @gaocegege @johnugeorge

Copy link
Contributor

Choose a reason for hiding this comment

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

@andreyvelich ok sure

Copy link
Member

Choose a reason for hiding this comment

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

@prem0912 Do you need @sperlingxx email to add him to the project ?

Copy link
Member

Choose a reason for hiding this comment

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

@sperlingxx Did you register in pypi ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@andreyvelich Yes, I registered with email lovedreamf@gmail.com and user name sperlingxx.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sperlingxx added in kubeflow-katib project

Copy link
Member

Choose a reason for hiding this comment

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

@sperlingxx Can you upload the package and we can merge this PR, please?

Copy link
Member Author

Choose a reason for hiding this comment

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

@andreyvelich @prem0912 It's done, please visit https://pypi.org/project/kubeflow-katib/0.0.3 . And I had also made some fixes during packaging and uploading.

"import kubeflow.katib as kc\n",
"from kubeflow.katib import constants\n",
"from kubeflow.katib import utils\n",
"from kubeflow.katib import V1alpha3AlgorithmSetting\n",
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to change it to v1beta1?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed v1beta1 examples temporarily because I met some obstacles on setup environments for these notebook examples.

Copy link
Member

@andreyvelich andreyvelich Aug 11, 2020

Choose a reason for hiding this comment

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

Ok.
@sperlingxx can you create an issue to add examples for v1beta1 SDK, please?

Copy link
Member Author

Choose a reason for hiding this comment

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

@andreyvelich It's Done #1300 .

sdk/python/v1alpha3/docs/V1Time.md Show resolved Hide resolved
@gaocegege
Copy link
Member

/lgtm
/assign @johnugeorge @andreyvelich @jinchihe

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 17, 2020
@sperlingxx
Copy link
Member Author

/retest

@gaocegege
Copy link
Member

/assign @andreyvelich @johnugeorge @jinchihe

Request another review, thanks.

@andreyvelich
Copy link
Member

Thank you @sperlingxx!
Do you need to modify README for v1beta1: https://github.com/sperlingxx/katib/tree/add_v1beta1_python_models/sdk/python/v1beta1#documentation-for-models?
It still points on v1alpha3 models.

@sperlingxx
Copy link
Member Author

Thank you @sperlingxx!
Do you need to modify README for v1beta1: https://github.com/sperlingxx/katib/tree/add_v1beta1_python_models/sdk/python/v1beta1#documentation-for-models?
It still points on v1alpha3 models.

@andreyvelich Done!

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thank you for doing this @sperlingxx.
/lgtm
/approve

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich

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:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants