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

Refactor README #1667

Merged

Conversation

andreyvelich
Copy link
Member

Fixes: #1570.

I made some changes in the README to leave only necessary information for our users.

Few questions:

  1. How do you feel about Algorithm table in this PR with HP, NAS and EarlyStopping headers ?
  2. Do we want to add implementation in the Algorithm table with type of framework that we use (e.g. hyperopt, optuna, goptuna) ?
  3. Any other information that we can add in the README (e.g. Why Katib ?) ?

Please take a look and give your feedback @gaocegege @johnugeorge @kimwnasptd @jbottum @RFMVasconcelos @anencore94 @tenzen-y @c-bata @g-votte

@google-oss-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

Copy link
Member

@anencore94 anencore94 left a comment

Choose a reason for hiding this comment

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

Table looks nice :)

README.md Outdated
Comment on lines 157 to 158
Learn more about various Katib installs in the
[Kubeflow guide](https://www.kubeflow.org/docs/components/katib/hyperparameter/#katib-setup).
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about moving this sentence to right after #Installation, and add explanation following Installation is for stand-alone or for quick-start ?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a good point, thanks @anencore94

Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

Thanks for adding the supported K8s version to this documentation.

It might be better to use the kustomize binary instead of the built-in kustomize, since the kustomize version used by each user may be very different, and the built-in kustomize does not immediately bug fixes.

What do you think @andreyvelich ?

For the specific Katib release (for example `v0.11.1`) run this command:

```
kubectl apply -k "github.com/kubeflow/katib.git/manifests/v1beta1/installs/katib-standalone?ref=v0.11.1"

This comment was marked as resolved.

For the latest Katib version run this command:

```
kubectl apply -k "github.com/kubeflow/katib.git/manifests/v1beta1/installs/katib-standalone?ref=master"

This comment was marked as resolved.


- Kubernetes >= 1.17
- `kubectl` >= 1.21

This comment was marked as resolved.

@andreyvelich
Copy link
Member Author

Thank you for the review @tenzen-y.

It might be better to use the kustomize binary instead of the built-in kustomize, since the kustomize version used by each user may be very different, and the built-in kustomize does not immediately bag fixes.

Before the installation we have the requirements that user should install kubectl >= 1.21 to deploy Katib.
Should we make sure that our Standalone kustomize manifest is compatible with that version of kubectl ?

That will remove additional step for the new users to install kustomize binary.

For most advance users they can always follow the Kubeflow Katib guide to manipulate kustomize manifests.

WDYT @tenzen-y @gaocegege @johnugeorge ?

@tenzen-y
Copy link
Member

@andreyvelich

For most advance users they can always follow the Kubeflow Katib guide to manipulate kustomize manifests.

It sounds good. I was relieved to see that the detailed installation instructions are still available.

README.md Outdated

Katib supports Python SDK:
- TODO (andreyvelich): Add Katib components section from the website.
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove this till we add this section?

Copy link
Member Author

Choose a reason for hiding this comment

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

@johnugeorge
Copy link
Member

One thing that we have to add is , Why Katib is a better choice ? We can take this up later

@andreyvelich
Copy link
Member Author

One thing that we have to add is , Why Katib is a better choice ? We can take this up later

Sure, I created issue to track this.

@andreyvelich
Copy link
Member Author

/retest

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.

/lgtm

@gaocegege
Copy link
Member

/hold

@johnugeorge
Copy link
Member

/lgtm

@andreyvelich
Copy link
Member Author

/hold cancel

@google-oss-robot google-oss-robot merged commit 23a3a8a into kubeflow:master Sep 24, 2021
@andreyvelich andreyvelich deleted the issue-1570-refactor-readme branch September 24, 2021 11:14
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.

Refactoring README information
6 participants