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

[Front end] [FR] Better error message for custom metric viz. #3499

Open
numerology opened this issue Apr 13, 2020 · 14 comments
Open

[Front end] [FR] Better error message for custom metric viz. #3499

numerology opened this issue Apr 13, 2020 · 14 comments
Assignees

Comments

@numerology
Copy link

What steps did you take:

Actually it is reported by one of our friends from CAIP training team

He was trying to add custom pipeline metrics to one of the pipeline they're authoring and found it painful.

What happened:

The metrics UI did not show up. It turns out that the regex they're using is culprit. However the error message was not very helpful during the efforts.

It would be good to have a better and more informative debug messages.

Environment:

How did you deploy Kubeflow Pipelines (KFP)?

Kubeflow full-fledged deployment.

KFP version:

KFP SDK version:

Anything else you would like to add:

[Miscellaneous information that will assist in solving the issue.]

/kind feature

/area frontend
/priority p2

@numerology
Copy link
Author

cc @thebinaryone1

@numerology
Copy link
Author

/assign @Bobgy

This is not urgent but assign to you to triage :)

@Bobgy
Copy link
Contributor

Bobgy commented Apr 14, 2020

@numerology Thanks for report! Can you point me to where the regex is?

@Bobgy Bobgy added the status/triaged Whether the issue has been explicitly triaged label Apr 14, 2020
@Bobgy Bobgy added this to To do in KFP Post 1.0 Backlog via automation Apr 14, 2020
@thebinaryone1
Copy link

thebinaryone1 commented Apr 15, 2020

@Bobgy
Copy link
Contributor

Bobgy commented Apr 16, 2020

@thebinaryone1 Thanks!
Sure, we should add a better error message for this.

@Bobgy Bobgy added this to To do in KFP 1.0 via automation Apr 16, 2020
@Bobgy Bobgy removed this from To do in KFP Post 1.0 Backlog Apr 16, 2020
@Bobgy
Copy link
Contributor

Bobgy commented Apr 16, 2020

bumped priority

@Bobgy
Copy link
Contributor

Bobgy commented Apr 29, 2020

@amygdala was also hitting the issue and after a long time of troubleshooting, she finally found the root cause is this: #3309

@amygdala
Copy link
Contributor

See also: #3655

@Bobgy
Copy link
Contributor

Bobgy commented May 12, 2020

I tried to evaluate feasibility of adding the error message, but it seems it's probably easier to just fix #3655 than this.

The check is done in backend:

metricNamePattern = "^[a-z]([-a-z0-9]{0,62}[a-z0-9])?$"
.

UI only gets information about which metrics were successfully logged, it couldn't know which didn't. A proper implementation of the error message should let backend persist errors/warnings related to a run and let UI query that.

A hacky workaround is letting UI parse the mlpipeline metric artifact and verify if any metrics are not conformed. However, this cannot scale if backend changes implementation...

I'll try if raising the restriction can easily work by just changing the regex. If yes, let's go that route instead and hope fewer people will run into this.

@Bobgy
Copy link
Contributor

Bobgy commented May 12, 2020

Further updates will be tracked in #3655

@Ark-kun
Copy link
Contributor

Ark-kun commented May 12, 2020

The check is done in backend:

@hongye-sun Do you remember the reason for that check?

@Bobgy
Copy link
Contributor

Bobgy commented May 12, 2020

@Ark-kun We asked @hongye-sun in another issue report: #3309 (comment). There wasn't any implementation requirement, just resource naming alignment. So I'd prefer we remove this limitation.

@stale
Copy link

stale bot commented Aug 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the lifecycle/stale The issue / pull request is stale, any activities remove this label. label Aug 10, 2020
@Bobgy
Copy link
Contributor

Bobgy commented Aug 10, 2020

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen and removed lifecycle/stale The issue / pull request is stale, any activities remove this label. labels Aug 10, 2020
@Bobgy Bobgy added this to Needs triage in KFP Runtime Triage via automation Nov 18, 2020
@Bobgy Bobgy removed this from Backlog in KFP Post 1.0 Backlog Nov 18, 2020
@Bobgy Bobgy moved this from Needs triage to P2 in KFP Runtime Triage Nov 18, 2020
@Bobgy Bobgy assigned zijianjoy and unassigned Bobgy Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

7 participants