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

Model Searching #799

Merged
merged 19 commits into from
Apr 2, 2021
Merged

Model Searching #799

merged 19 commits into from
Apr 2, 2021

Conversation

anfee1
Copy link
Contributor

@anfee1 anfee1 commented Mar 29, 2021

Allowing users to search the ModelView Ui for a model via name, type, and version for the model.

Description

This PR creates a basic model view search experience for users to find a model quickly in comparison to looking through all of the models through the regular navigation system.

  • If this change is a backward incompatible change, why must this change be made?

#637

</div>
</TreeItem>
</TreeView>
<TreeItem nodeId="Models" label="Models">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than building a completely new UI, think about if it can be merged with an existing UI. Can you show models using the existing model list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2021-04-01 at 1 45 59 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new look I came up with for the Model Navigation system. I think it looks a little cleaner than the previous. Let me know if there's any modification that you think would make it look better.

central/src/main/webapp/components/ModelNavigator.jsx Outdated Show resolved Hide resolved
central/src/main/webapp/components/ModelNavigator.jsx Outdated Show resolved Hide resolved
@zachgk zachgk linked an issue Mar 31, 2021 that may be closed by this pull request
defaultCollapseIcon={<ExpandMoreIcon />}
defaultExpandIcon={<ChevronRightIcon />}
>
{modelZooData.map((application) => (
<TreeItem nodeId={application.key} label={application.title}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you keep the application trees when displaying the models? When having no search filters, which is probably the most common way users are going to use this page, it helps a lot in organizing the full list of models.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have now made it to where if there is no search filters are being used it will display the model list with the application trees and if it is filtered it will begin to show each model. Hopefully that addresses this comment.

<TreeItem nodeId={'Models'} label={'Models'}></TreeItem>
<div>
{modelList.map(application => (
console.log(application),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug statement

@@ -86,25 +98,117 @@ export default function ModelNavigator(props) {
const modelZooData = useFetch(URL);

const [model, setModel] = useState(null);
const [modelList, setModelList] = useState([]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the modelLists default value from [] into modelZooData. That way, users start in the page having models to look at before being able to search to help find the model they want.

Shixiaowei02 and others added 3 commits April 2, 2021 10:54
Currently, the convolution testing tests with a range of inputs across a number
of variables. This increases the test duration a bit much for the standard unit
test suite.

Instead, I reduce down to only two tests for unit testing. During nightly, the
full combination of ranges will be used.

Change-Id: I3b9671e2c353315368d5dff07271607cc6917c4c
@zachgk
Copy link
Contributor

zachgk commented Apr 2, 2021

Please rebase and resolve conflicts

…odelSearching

# Conflicts:
#	central/src/main/webapp/components/ModelNavigator.jsx
@codecov-io
Copy link

Codecov Report

Merging #799 (bb7ee19) into master (c317884) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #799      +/-   ##
============================================
- Coverage     70.32%   70.28%   -0.04%     
+ Complexity     4970     4967       -3     
============================================
  Files           486      486              
  Lines         21793    21793              
  Branches       2263     2263              
============================================
- Hits          15325    15318       -7     
- Misses         5273     5278       +5     
- Partials       1195     1197       +2     
Impacted Files Coverage Δ Complexity Δ
...erving/src/main/java/ai/djl/serving/Arguments.java 87.75% <ø> (ø) 8.00 <0.00> (?)
...ving/src/main/java/ai/djl/serving/ModelServer.java 48.34% <ø> (ø) 11.00 <0.00> (?)
...rc/main/java/ai/djl/serving/ServerInitializer.java 100.00% <ø> (ø) 5.00 <0.00> (?)
.../java/ai/djl/serving/http/BadRequestException.java 50.00% <ø> (ø) 1.00 <0.00> (?)
...ava/ai/djl/serving/http/DescribeModelResponse.java 92.15% <ø> (ø) 19.00 <0.00> (?)
...c/main/java/ai/djl/serving/http/ErrorResponse.java 87.50% <ø> (ø) 3.00 <0.00> (?)
...n/java/ai/djl/serving/http/HttpRequestHandler.java 78.26% <ø> (ø) 6.00 <0.00> (?)
...a/ai/djl/serving/http/InferenceRequestHandler.java 52.38% <ø> (ø) 16.00 <0.00> (?)
...a/ai/djl/serving/http/InternalServerException.java 0.00% <ø> (ø) 0.00 <0.00> (?)
...ava/ai/djl/serving/http/InvalidRequestHandler.java 100.00% <ø> (ø) 2.00 <0.00> (?)
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c317884...bb7ee19. Read the comment docs.

@zachgk zachgk merged commit 7f44527 into deepjavalibrary:master Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ModelView Model Searching
6 participants