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 25 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.
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.
I'm not sure if this is the cause of your storybook problems, but it seems like the code you are adding should not be relying on applab like this. Looking at the chain of dependencies, this is going to make it so that every level type which uses CodeWorkspace is now going to be importing all of App Lab. One idea would be to break this dependency, and then see if you still have the same storybook problems. Please let me know if you need more info on how to accomplish this.
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 guess I'll offer some info anyway :-) One way would be to "lazy load" the ModelManagerDialog, like this:
code-dot-org/apps/src/code-studio/assets/show.js
Line 7 in 5bd80f4
there may also be a simpler way to cut out the dependencies, but it might take a bit more understanding of your code than I currently have to figure out where.
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.
aha! yes, this import of Applab does seem to be the culprit, though I still don't understand the full intricacies of how. Thank you! I'll give lazy loading a try.
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.
@davidsbailey Like this? My Storybook issues are solved with this change! Is there anything else?
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-02.at.10.07.04.PM.mov
Hmmm... actually, we probably don't want to lazy load this? Or maybe there's a way to solve the dependency issue without the loading spinner?
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.
yes, that code looks correct. you might be able to configure the
loadable
library to not show the spinner. if that doesn't fix your problems, then there should be another way to break the dependency -- one way would be to pass a callback to the model manager which calls into applab, rather than hard-coding it to call into applab. or apply a similar strategy at another point in the chain of dependencies.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.