-
Notifications
You must be signed in to change notification settings - Fork 455
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
[python sdk] add v1beta1 models #1252
Conversation
There was a problem hiding this 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?
There was a problem hiding this 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.
I think it works. |
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. |
@gaocegege @sperlingxx That's huge, I will take a look as soon as possible. Great contribution! |
There was a problem hiding this 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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Why do you want to delete v1alpha3 version of SDK? Can we support 2 versions: |
Okay, I'll add v1alpha3 back. |
Thanks @sperlingxx. What do you think about folder structure proposed here: #1252 (review) with Also, is it possible to handle 2 versions in pip and what do we need for that? |
@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. |
Since users of |
SGTM @sperlingxx. |
Any updates? |
@sperlingxx Do you have plans do create |
I have created separate folders for each version. |
/retest |
@sperlingxx You need to rebase to pass the tests. |
369f22e
to
e68598e
Compare
@andreyvelich Done. |
### pip install | ||
|
||
```sh | ||
pip install kubeflow-katib |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreyvelich ok sure
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 .
/lgtm |
/retest |
/assign @andreyvelich @johnugeorge @jinchihe Request another review, thanks. |
Thank you @sperlingxx! |
@andreyvelich Done! |
There was a problem hiding this 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
[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 |
What's your opinion about this PR, @prem0912 @andreyvelich @gaocegege @johnugeorge ?