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

Pull Request #1176 broke backward compatibility #1186

Closed
sperlingxx opened this issue May 12, 2020 · 10 comments · Fixed by #1189
Closed

Pull Request #1176 broke backward compatibility #1186

sperlingxx opened this issue May 12, 2020 · 10 comments · Fixed by #1189

Comments

@sperlingxx
Copy link
Member

sperlingxx commented May 12, 2020

/kind bug

What steps did you take and what happened:
The refactoring of metric schema in this commit made the schema of TrialStatus.Observation incompatible with legacy trials (because float metric value in legacy trials can't be parsed as string value). So, the trial controller will crash after updating to latest master.

What did you expect to happen:
In my opinion, we had better to fix this problem. Otherwise, users have to evict all existing trials before updating katib. Maybe we can keep the original float64 field value, and add another string field like non_numeric_value (which stores non-numeric metric value). If fieldnon_numeric_value is set, then take value of this field as metric value; If not, take value of original field value.

What do you think ? @andreyvelich @gaocegege

@issue-label-bot
Copy link

Issue Label Bot is not confident enough to auto-label this issue.
See dashboard for more details.

1 similar comment
@issue-label-bot
Copy link

Issue Label Bot is not confident enough to auto-label this issue.
See dashboard for more details.

@gaocegege
Copy link
Member

How about creating a new version v1beta1 @johnugeorge @andreyvelich

@andreyvelich
Copy link
Member

Good point @sperlingxx.

I am not sure that we should duplicate parameters in API. That can increase API complexity.
Currently, we don't have stable Katib API and versions update mechanism, Katib is in alpha version.
I think, if users don't want to use Kubeflow 1.0 version with 0.8 Katib version and use latest Katib release, they should restart running Experiment or they can have more errors.

I am ok with reverting this commit from upstream now and push it to the new version (v1beta1).

What do you think @johnugeorge @gaocegege ?

@issue-label-bot
Copy link

Issue-Label Bot is automatically applying the labels:

Label Probability
area/katib 0.68

Please mark this comment with 👍 or 👎 to give our bot feedback!
Links: app homepage, dashboard and code for this bot.

@gaocegege
Copy link
Member

SGTM

@sperlingxx
Copy link
Member Author

@andreyvelich The approach ( "reverting this commit from upstream now and push it to the new version (v1beta1)" ) looks great. And when we setup v1beta1?

@andreyvelich
Copy link
Member

@johnugeorge @gaocegege Do we want to try to resolve some p0/p1 tasks before new release?

@gaocegege
Copy link
Member

I think we can do it now and start developing on v1beta1. WDYT @johnugeorge

@johnugeorge
Copy link
Member

Agree. @gaocegege Lets work on v1beta1 api and get all new api changes in v1beta1

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

Successfully merging a pull request may close this issue.

5 participants