Skip to content
This repository has been archived by the owner on Sep 12, 2023. It is now read-only.

Revert ReplicaType to string #173

Merged
merged 1 commit into from
Nov 22, 2021
Merged

Conversation

zw0610
Copy link
Member

@zw0610 zw0610 commented Nov 10, 2021

Since PR #163 has been stalled for a while, this pr tries to accomplish the same goal: revert rtype from commonv1.ReplicaType to string.

For any parameters, if it is of type commonv1.ReplicaType, it's reverted to string in this pr. However, parameters of type map[commonv1.ReplicaType]{any_value_type} remain unchanged.

@Jeffwan

@zw0610 zw0610 changed the title WIP: revert ReplicaType to string Revert ReplicaType to string Nov 10, 2021
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

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

/assign @Jeffwan

@zw0610
Copy link
Member Author

zw0610 commented Nov 12, 2021

@Jeffwan As tested in this pr kubeflow/training-operator#1462, the updated common library works with training-operator.

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

@Jeffwan
Copy link
Member

Jeffwan commented Nov 19, 2021

This PR can move forward once the issue here kubeflow/training-operator#1462 is addressed.

@zw0610
Copy link
Member Author

zw0610 commented Nov 21, 2021

ping @Jeffwan @terrytangyuan as the all tested passed: kubeflow/training-operator#1462

However, please note that tf-controller is actually modified to keep up with the labels in new kubeflow/common. I submitted another pr kubeflow/training-operator#1474 to fix the label issue.

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

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Thanks!

/lgtm
/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: terrytangyuan

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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants