APIs for retrieving models in parallel #199
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jennuine
reviewed
Aug 31, 2021
src/FuelClient.cc
Outdated
| if (!this->CachedModel(dependencyURI, dependencyPath)) | ||
| this->DownloadModel(dependencyURI, dependencyPath); | ||
| ModelIdentifier dependencyID; | ||
| this->ParseModelUrl(dependencyURI, dependencyID); |
There was a problem hiding this comment.
Should we check if parsing failed?
src/FuelClient.cc
Outdated
| } | ||
| } | ||
| } | ||
|
|
||
| return Result(ResultType::FETCH); | ||
| } | ||
|
|
||
| ////////////////////////////////////////////////// | ||
| Result FuelClient::DownloadModels( |
There was a problem hiding this comment.
Is this function not finished? It doesn't appear to be doing anything and isn't being used
| const std::vector<std::string> &_headers, | ||
| std::vector<ModelIdentifier> &_dependencies); | ||
|
|
||
| public: Result DownloadModels( |
| const std::vector<ModelIdentifier> &_ids, | ||
| size_t _jobs = 2); | ||
|
|
||
| /// \brief Retrieve the list of dependencies for a model. |
There was a problem hiding this comment.
codecheck error: remove line ending whitespace
Suggested change
| /// \brief Retrieve the list of dependencies for a model. | |
| /// \brief Retrieve the list of dependencies for a model. |
src/FuelClient.cc
Outdated
| if(!res) | ||
| return res; | ||
|
|
||
| for (auto dep: dependencies) |
There was a problem hiding this comment.
codecheck error
Suggested change
| for (auto dep: dependencies) | |
| for (auto dep : dependencies) |
src/FuelClient.cc
Outdated
| @@ -678,10 +700,19 @@ Result FuelClient::DownloadModel(const ModelIdentifier &_id, | |||
| if (!this->dataPtr->cache->SaveModel(newId, resp.data, true)) | |||
| return Result(ResultType::FETCH_ERROR); | |||
|
|
|||
| return this->ModelDependencies(_id, _dependencies); | |||
There was a problem hiding this comment.
codecheck error
Suggested change
| return this->ModelDependencies(_id, _dependencies); | |
| return this->ModelDependencies(_id, _dependencies); |
Signed-off-by: Michael Carroll <michael@openrobotics.org>
mjcarroll
force-pushed
the
parallel_downloads
branch
from
27ceec8 to
1b3595c
Compare
|
Does this resolve #188 for you? I still see the errors when using a command like this: |
Signed-off-by: Michael Carroll <michael@openrobotics.org>
|
I'm getting similar errors seen in osrf/subt#943 when running such as |
Signed-off-by: Michael Carroll <michael@openrobotics.org>
|
Updated the implementation to use threads rather than async which seems to avoid the segfaults I was seeing before. |
|
I'm seeing compiler errors, |
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
|
@mjcarroll , I think you were missing this line: d16d499? Can you confirm? |
Signed-off-by: Michael Carroll <michael@openrobotics.org>
…s/ign-fuel-tools into parallel_downloads
Codecov Report
@@ Coverage Diff @@
## ign-fuel-tools4 #199 +/- ##
===================================================
- Coverage 78.13% 77.92% -0.21%
===================================================
Files 19 19
Lines 2643 2718 +75
===================================================
+ Hits 2065 2118 +53
- Misses 578 600 +22
Continue to review full report at Codecov.
|
nkoenig
approved these changes
Nov 5, 2021
|
fix for windows build in #213 |
scpeters
reviewed
Nov 7, 2021
| auto result = client.DownloadWorlds(worldIds, _jobs); | ||
| ignerr << "Failed to download worlds for collection [" | ||
| << collection.Name() | ||
| << "]" << std::endl; |
There was a problem hiding this comment.
is this error message always printed even if the worlds are downloaded successfully?
scpeters
added a commit
that referenced
this pull request
Nov 7, 2021
This was introduced in #199. Signed-off-by: Steve Peters <scpeters@openrobotics.org>
mjcarroll
pushed a commit
that referenced
this pull request
Nov 8, 2021
* FuelClient.cc: include <deque> Fixes the Windows build. Signed-off-by: Steve Peters <scpeters@openrobotics.org> * ign_src_TEST: print error messages Signed-off-by: Steve Peters <scpeters@openrobotics.org> * print one more error message Signed-off-by: Steve Peters <scpeters@openrobotics.org> * ign.cc: remove console error message This was introduced in #199. Signed-off-by: Steve Peters <scpeters@openrobotics.org>
|
This pull request has been mentioned on Gazebo Community. There might be relevant details there: https://community.gazebosim.org/t/new-ignition-releases-2022-01-10/1228/1 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 feature
Closes #188
Summary
Adds APIs for downloading a set of models in parallel.
Test it
Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge