-
Notifications
You must be signed in to change notification settings - Fork 25
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
graphql requests module #244
Conversation
license name change
Co-authored-by: dhuang <[email protected]>
993f24b
to
92b8c83
Compare
search search, download draft draft, successful search and download draft Update: `ModelAnalysis.from_onnx(...)` to additionally work with loaded `ModelProto` (#253) refactor search, download
49cb39a
to
c4426e9
Compare
b8f41b4
to
e069283
Compare
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.
Left some tiny comments.
In general, as a reviewer I would love to have this split to one big feature branch and smaller PRs (much easier to review). Also, wouldn't hurt to leave more verbose docstrings.
Thanks for the comments. A good reason for the length of the 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.
Nothing glaring sticks out to me, though I would be interested to know if there's a good reason for making multiple graphql requests for a single model
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.
Looks great! Made a couple suggestions on structure, most are nits though. Left one comment about the sparse_tag, that's the only thing I saw that looked potentially wrong
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.
Nice, looks good to me! 👍
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.
LGTM
Use v2 api endpoints for search and downloads.
Please checkout the graphiql interactive UI to play around with the apis.
https://staging-api.neuralmagic.com/v2/graphql
Example query: