-
Notifications
You must be signed in to change notification settings - Fork 6
Allows binary download for edge #313
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
Conversation
paulina-positronix
left a comment
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.
It seems like the main change is inside experimental_api.py which LGTM. I'm not sure about the rest of the generated API code and especially the empty tests.
|
|
||
| def testEdgeModelInfo(self): | ||
| """Test EdgeModelInfo""" | ||
| # FIXME: construct object with mandatory attributes with example values |
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.
Is this supposed to be replaced with an actual test?
| pass | ||
|
|
||
| def test_get_model_urls(self): | ||
| """Test case for get_model_urls""" |
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.
Is this supposed to be replaced with an actual test?
|
|
||
| def testModelUrl(self): | ||
| """Test ModelUrl""" | ||
| # FIXME: construct object with mandatory attributes with example values |
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.
Is this supposed to be replaced with an actual test?
| def _download_mlbinary_url(self, detector: Union[str, Detector]) -> EdgeModelInfo: | ||
| """ | ||
| Gets a temporary presigned URL to download the model binaries for the given detector, along | ||
| with relevant metadata |
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 you need secrets or anything to do this? Or is that taken care of 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.
The secrets are taken care of in the backend after you've authenticated with your api token. So long as you have the feature enabled in your Groundlight account, you should be able to download
True that the generated tests aren't particularly useful. Nothing inside the generated folder is written by hand, and in fact editing those files manually is very likely to get those changes overwritten, which is why we don't update the tests there. |
No description provided.