Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[AiLab] Model Manager Dialog and autogenerated design elements and code #38664
[AiLab] Model Manager Dialog and autogenerated design elements and code #38664
Changes from 35 commits
5af4d1e
4ed5c30
cd853b3
a018f1c
1a196bf
fe30108
74a8b96
7360835
c2b38fc
1b931ab
5387166
8ee19f7
a4ad3dc
a693b39
db7ce74
14a6519
568be9e
a92018d
26a9139
c561c6b
4425ebc
05f8cca
4354b6c
f2f07e5
6fd6500
72a2943
57878c8
2348784
30d1396
7b91f3b
52cf24b
15f5245
bf54c02
c313d29
90a6371
5a422f1
5144b3d
c2c4983
96f869c
d2e4a97
3827a44
bea44e7
66b4898
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to expose errors in prediction to the user writing an app? (Both so that they can debug, and also so that they can give information back to the user running their app.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it so that we're displaying the error if the prediction fails; do this seem reasonable? At least until we get more feedback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like a good approach for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a lot of "ml" related naming, but it sounds like we are going to trend towards "ai" instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - it'd be good to get all on the same page about that and update the code to match. Ok to do as a rename/refactor after this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this dialog will live next to and be most similar to these two components, so just linking those here in case it's helpful:
<LibraryManagerDialog/>
<AssetManager/>
The LibraryManagerDialog is newer and more consistent with our newer styles, so i'd go with that one if you're trying to decide how to style something (related to our convo here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running a regular AppLab app, we keep on hitting
/api/v1/ml_models/names
. This component seems to be updating more than expected.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
componentDidMount()
instead ofcomponentDidUpdate()
🤦♀️There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems to fire once, but due to the lifetime management of these dialogs (they live forever, just showing and hiding), it doesn't refresh the model list each time the dialog is opened. I have implemented what seems to be a solution in #39041.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this function does most of its work asynchronously, should we show a spinner or something similar while it does its work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screen.Recording.2021-02-01.at.9.29.13.PM.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it does seem we might want to expose this error to the user's program, so they can then report it back to their end-user.