APIs for retrieving models in parallel #199

mjcarroll · 2021-08-16T19:38:21Z

🎉 New feature

Closes #188

Summary

Adds APIs for downloading a set of models in parallel.

Test it

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

jennuine

Looks good so far, appears to be still be in progress. Can this be retarged to Citadel?

jennuine · 2021-08-31T17:13:27Z

src/FuelClient.cc

-        if (!this->CachedModel(dependencyURI, dependencyPath))
-          this->DownloadModel(dependencyURI, dependencyPath);
+        ModelIdentifier dependencyID;
+        this->ParseModelUrl(dependencyURI, dependencyID);


Should we check if parsing failed?

jennuine · 2021-08-31T17:24:36Z

src/FuelClient.cc

      }
    }
  }

  return Result(ResultType::FETCH);
 }

+//////////////////////////////////////////////////
+Result FuelClient::DownloadModels(


Is this function not finished? It doesn't appear to be doing anything and isn't being used

jennuine · 2021-08-31T17:26:51Z

include/ignition/fuel_tools/FuelClient.hh

+                  const std::vector<std::string> &_headers,
+                  std::vector<ModelIdentifier> &_dependencies);
+
+      public: Result DownloadModels(


Missing docs

jennuine · 2021-08-31T17:41:23Z

include/ignition/fuel_tools/FuelClient.hh

+                  const std::vector<ModelIdentifier> &_ids,
+                  size_t _jobs = 2);
+
+      /// \brief Retrieve the list of dependencies for a model. 


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.

jennuine · 2021-08-31T17:41:57Z

src/FuelClient.cc

+  if(!res)
+    return res;
+
+  for (auto dep: dependencies)


codecheck error

Suggested change

for (auto dep: dependencies)

for (auto dep : dependencies)

jennuine · 2021-08-31T17:42:48Z

src/FuelClient.cc

@@ -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); 


codecheck error

Suggested change

return this->ModelDependencies(_id, _dependencies);

return this->ModelDependencies(_id, _dependencies);

Signed-off-by: Michael Carroll <[email protected]>

nkoenig · 2021-10-01T21:17:41Z

Does this resolve #188 for you? I still see the errors when using a command like this:

ign fuel download -v 4 -j 8 --type model -u "https://fuel.ignitionrobotics.org/OpenRobotics/collections/SubT Tech Repo"

Signed-off-by: Michael Carroll <[email protected]>

nkoenig · 2021-10-19T16:13:20Z

I'm getting similar errors seen in osrf/subt#943 when running

ign fuel download -v 4 -j 8 --type model -u "https://fuel.ignitionrobotics.org/OpenRobotics/collections/SubT Tech Repo"

such as

[Err] [Zip.cc:159] Error opening: materials/textures/Ground_Roughness.jpg
[Err] [Zip.cc:159] Error opening: materials/textures/Ground_Roughness.png
[Err] [Zip.cc:159] Error opening: materials/textures/Light_Albedo.jpg
[Err] [Zip.cc:159] Error opening: materials/textures/Light_Albedo.png
[Err] [Zip.cc:159] Error opening: materials/textures/Light_Emissive.jpg
[Err] [Zip.cc:159] Error opening: materials/textures/Light_Emissive.png
[Err] [Zip.cc:159] Error opening: materials/textures/Light_Metalness.jpg
[Err] [Zip.cc:159] Error opening: materials/textures/Light_Metalness.png
[Err] [Zip.cc:159] Error opening: materials/textures/Light_Normal.jpg

Signed-off-by: Michael Carroll <[email protected]>

mjcarroll · 2021-10-25T19:07:43Z

Updated the implementation to use threads rather than async which seems to avoid the segfaults I was seeing before.

nkoenig · 2021-10-29T15:30:23Z

I'm seeing compiler errors,

/FuelClient.cc:22:
/usr/include/c++/8/bits/unordered_set.h: In constructor ‘std::unordered_set<_Value, _Hash, _Pred, _Alloc>::unordered_set(_InputIterator, _InputIterator, std::unordered_set<_Value, _Hash, _Pred, _Alloc>::size_type, const hasher&, const key_equal&, const allocator_type&) [with _InputIterator = __gnu_cxx::__normal_iterator<const ignition::fuel_tools::ModelIdentifier*, std::vector<ignition::fuel_tools::ModelIdentifier> >; _Value = ignition::fuel_tools::ModelIdentifier; _Hash = std::hash<ignition::fuel_tools::ModelIdentifier>; _Pred = std::equal_to<ignition::fuel_tools::ModelIdentifier>; _Alloc = std::allocator<ignition::fuel_tools::ModelIdentifier>; std::unordered_set<_Value, _Hash, _Pred, _Alloc>::size_type = long unsigned int; std::unordered_set<_Value, _Hash, _Pred, _Alloc>::hasher = std::hash<ignition::fuel_tools::ModelIdentifier>; std::unordered_set<_Value, _Hash, _Pred, _Alloc>::key_equal = std::equal_to<ignition::fuel_tools::ModelIdentifier>; std::unordered_set<_Value, _Hash, _Pred, _Alloc>::allocator_type = std::allocator<ignition::fuel_tools::ModelIdentifier>]’:
/usr/include/c++/8/bits/unordered_set.h:168:30: error: use of deleted function ‘std::hash<ignition::fuel_tools::ModelIdentifier>::hash()’
         const hasher& __hf = hasher(),

Signed-off-by: Michael Carroll <[email protected]>

Signed-off-by: Nate Koenig <[email protected]>

nkoenig · 2021-10-29T21:58:21Z

@mjcarroll , I think you were missing this line: d16d499? Can you confirm?

Signed-off-by: Michael Carroll <[email protected]>

…s/ign-fuel-tools into parallel_downloads

codecov · 2021-11-04T21:59:46Z

Codecov Report

Merging #199 (a66b85f) into ign-fuel-tools4 (d078bd0) will decrease coverage by 0.20%.
The diff coverage is 81.19%.

@@                 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

Impacted Files	Coverage Δ
src/FuelClient.cc	`73.23% <80.35%> (+0.87%)`	⬆️
src/ign.cc	`57.74% <100.00%> (-3.13%)`	⬇️

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 d078bd0...a66b85f. Read the comment docs.

scpeters · 2021-11-06T15:42:13Z

fix for windows build in #213

scpeters · 2021-11-07T05:43:12Z

src/ign.cc

+      auto result = client.DownloadWorlds(worldIds, _jobs);
+      ignerr << "Failed to download worlds for collection ["
+          << collection.Name()
+          << "]" << std::endl;


is this error message always printed even if the worlds are downloaded successfully?

This was introduced in #199. Signed-off-by: Steve Peters <[email protected]>

* FuelClient.cc: include <deque> Fixes the Windows build. Signed-off-by: Steve Peters <[email protected]> * ign_src_TEST: print error messages Signed-off-by: Steve Peters <[email protected]> * print one more error message Signed-off-by: Steve Peters <[email protected]> * ign.cc: remove console error message This was introduced in #199. Signed-off-by: Steve Peters <[email protected]>

osrf-triage · 2022-01-10T21:46:24Z

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

github-actions bot added the 🔮 dome Ignition Dome label Aug 16, 2021

nkoenig self-requested a review August 16, 2021 20:26

nkoenig requested a review from jennuine August 30, 2021 20:03

jennuine reviewed Aug 31, 2021

View reviewed changes

APIs for retrieving models in parallel

1b3595c

Signed-off-by: Michael Carroll <[email protected]>

mjcarroll force-pushed the parallel_downloads branch from 27ceec8 to 1b3595c Compare September 27, 2021 19:58

mjcarroll changed the base branch from ign-fuel-tools5 to ign-fuel-tools4 September 27, 2021 19:58

Lint

9520a6f

Signed-off-by: Michael Carroll <[email protected]>

Update to use parallel download models and worlds

032fa40

Signed-off-by: Michael Carroll <[email protected]>

Alternative implementation using threads

e01337e

Signed-off-by: Michael Carroll <[email protected]>

mjcarroll marked this pull request as ready for review October 25, 2021 19:07

mjcarroll and others added 3 commits October 29, 2021 11:41

Uncommited local changes for hash and linting

adf7898

Signed-off-by: Michael Carroll <[email protected]>

fix codecheck

358ede9

Signed-off-by: Nate Koenig <[email protected]>

Added missing insertion

d16d499

Signed-off-by: Nate Koenig <[email protected]>

mjcarroll added 2 commits November 4, 2021 16:52

Correctly handle local dependencies by skipping

31c879e

Signed-off-by: Michael Carroll <[email protected]>

Merge branch 'parallel_downloads' of ssh://github.com/ignitionrobotic…

a66b85f

…s/ign-fuel-tools into parallel_downloads

nkoenig approved these changes Nov 5, 2021

View reviewed changes

nkoenig merged commit c680ee8 into ign-fuel-tools4 Nov 5, 2021

nkoenig deleted the parallel_downloads branch November 5, 2021 13:43

scpeters mentioned this pull request Nov 6, 2021

FuelClient.cc: include <deque> #213

Merged

7 tasks

scpeters reviewed Nov 7, 2021

View reviewed changes

scpeters mentioned this pull request Nov 7, 2021

redirectIO helper function in ign_src_TEST.cc doesn't work on all platforms #214

Open

scpeters added a commit that referenced this pull request Nov 7, 2021

ign.cc: remove console error message

821f8da

This was introduced in #199. Signed-off-by: Steve Peters <[email protected]>

mjcarroll mentioned this pull request Nov 8, 2021

Threaded collection download with model dependencies causes errors #188

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APIs for retrieving models in parallel #199

APIs for retrieving models in parallel #199

mjcarroll commented Aug 16, 2021

jennuine left a comment

jennuine Aug 31, 2021

jennuine Aug 31, 2021

jennuine Aug 31, 2021

jennuine Aug 31, 2021

jennuine Aug 31, 2021

jennuine Aug 31, 2021

nkoenig commented Oct 1, 2021

nkoenig commented Oct 19, 2021

mjcarroll commented Oct 25, 2021

nkoenig commented Oct 29, 2021

nkoenig commented Oct 29, 2021

codecov bot commented Nov 4, 2021 •

edited

Loading

scpeters commented Nov 6, 2021

scpeters Nov 7, 2021

osrf-triage commented Jan 10, 2022

	/// \brief Retrieve the list of dependencies for a model.
	/// \brief Retrieve the list of dependencies for a model.

	return this->ModelDependencies(_id, _dependencies);
	return this->ModelDependencies(_id, _dependencies);

APIs for retrieving models in parallel #199

APIs for retrieving models in parallel #199

Conversation

mjcarroll commented Aug 16, 2021

🎉 New feature

Summary

Test it

Checklist

jennuine left a comment

Choose a reason for hiding this comment

jennuine Aug 31, 2021

Choose a reason for hiding this comment

jennuine Aug 31, 2021

Choose a reason for hiding this comment

jennuine Aug 31, 2021

Choose a reason for hiding this comment

jennuine Aug 31, 2021

Choose a reason for hiding this comment

jennuine Aug 31, 2021

Choose a reason for hiding this comment

jennuine Aug 31, 2021

Choose a reason for hiding this comment

nkoenig commented Oct 1, 2021

nkoenig commented Oct 19, 2021

mjcarroll commented Oct 25, 2021

nkoenig commented Oct 29, 2021

nkoenig commented Oct 29, 2021

codecov bot commented Nov 4, 2021 • edited Loading

Codecov Report

scpeters commented Nov 6, 2021

scpeters Nov 7, 2021

Choose a reason for hiding this comment

osrf-triage commented Jan 10, 2022

codecov bot commented Nov 4, 2021 •

edited

Loading