-
Notifications
You must be signed in to change notification settings - Fork 131
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
Add modelzoo metadata, improve integrity #1200
base: v3_develop
Are you sure you want to change the base?
Conversation
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.
Thanks Jakub!
Generally looks good, left just a couple of comments.
/** | ||
* @brief Download model from model zoo | ||
*/ | ||
void downloadModel(); | ||
void downloadModel(const nlohmann::json& responseJson); |
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.
Perhaps nicer to return the object instead since the function is now void anyhow.
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.
Are you suggesting downloadModel
returns a path to the downloaded file?
src/modelzoo/Zoo.cpp
Outdated
bool ZooManager::internetIsAvailable() const { | ||
constexpr int timeout_ms = 5000; | ||
constexpr std::string_view host = "http://example.com"; | ||
try { | ||
cpr::Response r = cpr::Get(cpr::Url{host}, cpr::Timeout{timeout_ms}); | ||
return r.status_code == cpr::status::HTTP_OK; | ||
} catch(const std::exception& e) { | ||
return false; | ||
} | ||
} |
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.
In case of no internet this will slow down startup by 5 seconds, I think we should reduce this to ~500ms instead.
Would it maybe make sense to try pinging the URL we actually need instead of http://example.com
?
Some customers might setup the network in a way where only the Zoo is accessible for the model updates, etc.
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 performed some empirical tests and 500 milliseconds turn out not to be enough. At times, depthai
would not receive a response from the hub fast enough and thus conclude that there is no internet connection despite my PC being connected to the network. A timeout of 1 second turns out to be enough.
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.
Turns out maybe even that 1 second is not sufficient. Changing to a two second timeout.
It might be of value do add a flag/environment variable that will disable this initial network checking. I would set it to True
by default but if a user is certain such a check is not needed, they might set it to False
.
Co-authored-by: moratom <[email protected]>
…if internet connection test failed
Clickup task: https://app.clickup.com/t/86c1bmx1r
This PR improves our functions for downloading models from the hub. A new
metadata.yaml
file is generated for each and every downloaded model for easier checking and hash validation.