-
Notifications
You must be signed in to change notification settings - Fork 29
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
DVC errors that may occur during or after running experiment #1636
Comments
@shcheklein let me know what do you think.
The hint including Error and How To Fix content example |
@maxagin we need to see a bit more realistic scenarios - e.g. the whole params file / metrics file could not be parsed (so many errors at once), or if the whole experiment failed - what do exactly we see instead of metrics / params / etc. It might change the requirements and perception of the design. |
@shcheklein here are a few examples. Let me know what you think about it (The modal opens on click) The whole metrics file could not be parsed (many errors at once) If the whole experiment failed |
Thanks, Max. It makes sense, but happens if we have 10+ values + 10 experiments? Think I'm trying to see here is how noisy it can become if we are using these full color icons. |
@maxagin that's good (not sure if it's easy or not to implement those sticky signs at the edge though). A few questions / comments:
|
This needs to be tested with the current design patterns. I just wanted to introduce the concept quickly. However this is a webview container, so we have a lot of freedom I think.
Let me create a better visualisation of the concept, because I feel it is not clear.
DVC and Studio use ‘!’ for errors. I will create an example. However the symbol may have advantages, not just because it is different then font or has two states (no fill, with fill), but also because it stands out more, but not too much. Let’s test the ‘!’. |
Concept of showing row's errors that are shown at the table, but does not currently at the viewport (horizontal scroll)Responsive behaviour Just a stylized better example Behavior of the right panel elements on scrollWhen the last error of the row showed up the side indicator will be removed. Same if we scroll left - as soon as the error disappears under the left window age, the indicator will appear at the right panel. If there are no hidden errors the right panel won't appear |
Using exclamation mark instead of rhombus for the errors symbol. |
Questions that we still have:
|
Please put the final design into #662 when it is ready and close this issue. |
What is the status of this, @maxagin ? |
@shcheklein I am preparing the Figma components. This is next on my list. Probably Monday will be done (first phase). The description has been updated. |
Hello. I have finalized the specs in Figma. Please review the doc and let me know if you have any comments/suggestions. @shcheklein I especially added the Overview section so we can see the final UI and have a reference before jumping to the specs details. Folks, let me know if this makes sense. Thanks The ticket description has also been updated. |
Looks good to me @maxagin , left a few small comments in Figma. |
@shcheklein, sharing the notifications examples below. However here we are talking about hints and our hints are using ‘gray’ colors. I would suggest sticking to the existing pattern. wdyt? |
they are grey now, because they are indeed "hints" / neutral, and they don't even have any titles, right? I would take exactly same style for errors as VC Code has, wdyt? Except in our case symlol can be |
My logic.
|
Sorry, may be it was not clear. I was asking only about tooltips, not about the table itself, and only about it's title "Error" (or we could use a small icon). I'm not sure we even had an intention to
Yes, I find it confusing. It's the case where certain style is expected. Error can't be green or yellow. It's either make it neutral or make it all some dark red- everything else won't work very well I think. |
Example no. 1 Example no. 2 @shcheklein let me know wdyt please |
Another option would be to have it all white everywhere. |
sorry, maybe I didn't communicate this well. I was totally fine to have them white in the table (and I think it's even better), I specifically meant only those colorful in Figma - only in the first column. I like the tooltip (icon is not fully aligned?, but otherwise it looks good). |
It's not an icon I believe, right? It's just a char. And we are using it for consistency (DVC CLI). Also, heavy icons in the table will create a lot of noise. |
In the tooltip it's fine to use the built-one, it won't be consistent with the table though. Your call. |
@mattseddon I have updated the specs taking into consideration our conversations. Please let me know if you find anything to be improved. If exp is broken we may want to use "..." for the cells to indicate - no information. This is not the error, but just no info indicator. WDYT? |
I think that the native icons you mentioned are related to the Notifications, which are placed at the specific right/button corner and have actions to resolve the issue + specific color. This is why I have created a special symbol (which actually was a part of the codicon set before update #1636 (comment)) |
Re: "special symbol" - From https://code.visualstudio.com/api/references/icons-in-labels: This seems very close to our use case. I would suspect that the icons was removed from the codicons or replaced for good reason. We should not bring it back. From the design:
|
This is planned to be fixed in the second phase.
If exp is broken we may want to use "..." for the cells to indicate - no information. This is not the error, but just no info indicator at the cell level.
Please explain. TMO what we have here it is prity strate forward. |
The same can be said for either situation. We have no data to show. The data from the CLI does not contain
Here are the rules as I see them If an experiment has a record level error:
If an experiment has a file level error:
As a user just looking at the table, I would have to look in different places to confirm that there are no errors for a particular record. Seems overcomplicated when we already have a sticky column that can contain all high-level information relating to the experiment. Tieing in with #1649 (comment)
|
Thumbs up @mattseddon for the details, this is partly completed already, however some needs to be addressed. Let me reflect on it and I will share the updated specs soon. |
Without “!” or “...” in the cells? Right? |
|
Hey. This is an updated overview including every detail. Please check it out and let me know if we are good here so I can create a full spec document. Thank you @mattseddon |
This doesn't answer a lot of my questions/concerns from above. We can discuss tomorrow in the planning meeting once the release is done. |
Very good @mattseddon ! |
@maxagin I would like to start working on this again, I looked at the spec and everything has been moved back into WIP. LMK when the next iteration is ready. |
Thank you @mattseddon for the heads up. I will try to finish it at the earliest convenience - there is still some work needed. |
@pmrowla Will there be any circumstance in which the whole experiment fails and still shows in That might no longer be relevant in this view (although it might be shown if VS Code adds a view of the queue). |
@mattseddon Does it mean we are not expecting to have any error at the row-level anymore? |
By default failed experiments will not show in the table after the queueing changes, but we have discussed adding flags to |
Hey folks! The new specs version was published. Please use it and let me know if you have any concerns. |
Provided feedback in the spec 👍🏻. |
@mattseddon the specs have been updated. Please review it and comment. Thanks! |
The first iteration has been done. Let's close this issue and come back to it when we work on plots errors (the two specs are together). |
Good. Thank you @mattseddon ! |
Find solution for displaying errors when the experiment failed, broken parameters, metrics and more.
TODO
EXPs table
Figma specs
Plots
Figma specs
To consider
Helpful links
The text was updated successfully, but these errors were encountered: