Add LoadMap service to map_server

[mkhansenbot]

---

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#239
Primary OS tested on	Ubuntu
Robotic platform tested on	gazebo simulation of turtlebot

---

Description of contribution in a few bullet points

Added a "LoadMap" service, which accepts a filename and loads the file into the server
Still To Do:

• Return fail codes (marked as TODO in code)

• Create a unit test

• Update documentation

---

Future work that may be required in bullet points

[SteveMacenski]

I know you're probably just back from ROSCon, but really you just need to publish the updated map and this can just be merged.

[mkhansenbot]

I'm going to do the 3 things in the 'to do' list above: fail codes, unit test and docs before I mark this ready for review

[mkhansenbot]

I don't understand this Circle CI fail:

Config Processing Error (Don't rerun)00:00
Exit code: 1

#!/bin/sh -eo pipefail
# Orb codecov/codecov@1.0.5 not loaded. To use this orb, an organization admin must opt-in to using third party orbs in Organization Security settings.
# 
# -------
# Warning: This configuration was auto-generated to show you the message above.
# Don't rerun this job. Rerunning will have no effect.
false
Exited with code 1

@crdelsey or @ruffsl if you know what this means please help

[SteveMacenski]

It may be draft PR related.

[ruffsl]

Looking at the link for the CI, it seems that draft PRs are executed under the PR owner's org:

https://circleci.com/gh/mkhansen-intel/navigation2/129?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Note the org is listed as mkhansen-intel. In the ros-planning org, we have already enabled 3rd party orbs in the CircleCi account settings. To run CI test on your own fork (and under your own individual org/user account), you'll have to similarly enable enabled 3rd party orbs for your fork's CI settings.

See my #990 (comment) here.

[mkhansenbot]

Closing - will re-open to see if CI works

[mkhansenbot]

I tried -

• changing from draft PR to PR
• updating my settings to allow 3rd party ORBs
• closing and re-opening the PR

None of those seem to work, I'm getting the same build error

[SteveMacenski]

Did you try rebasing?

[SteveMacenski]

Also, are you ready now for a review?

[mkhansenbot]

Just rebased and re-pushed. That seems to have re-triggered circleci. Fingers are crossed.

Assuming that passes, it's ready for review.

[SteveMacenski]

Small stuff

[mkhansenbot]

I think I've addressed all the feedback. Please take another look.

[jacobperron]

Let's try to coordinate with my ROS 1 PR (ros/common_msgs#152) to make bridging in the future more straight forward.

A path to the file can also be represented as a URL. I think doing something similar to what I've done in ros/common_msgs#152 leaves the door open to other locator types without having to modify the message definition.

[mkhansenbot]

jacob, how do you plan to get the YAML file from the URL? Are you planning to use curl or is there an easier way to load a YAML file directly from a URL?

[jacobperron]

I haven't thought about it too much. For the file URL, curl might work. As a first iteration, it might also be just as easy to write (or find) a simple URL parser to check if the prefix of the URL is file:// and then take the rest of the URL as the path to the YAML file.

To load a YAML from a package directory (eg. package://) we'll need URL parser anyways to get the package name and relative path, and then we can use the ament API to locate the file.

[jacobperron]

Actually, we should probably use the existing resource_retriever for the job. It it a curl-like tool that also supports package:// :)

I forgot about it.

[mkhansenbot]

OK, that's very helpful. I wasn't aware of resource_retriever.

I'll have to re-write part of this PR to use that and the message definition you created.

[SteveMacenski]

Looks good, coordinate with Jacob

[mkhansenbot]

So @SteveMacenski and @crdelsey - If I continue with the service and resource_retriever method that Jacob has proposed, existing map.yaml files will need to change to be compatible. See my comment here:
ros/common_msgs#152 (comment)

What are your thoughts on this?

[SteveMacenski]

This should also still take a relative or full path by default if no [thing]:// exists for backwards compatibility.

That would be a huge hindrance for anyone moving to ROS2 to have to change all their maps. Am I correct in reading that this (https://github.com/ros-planning/navigation2/pull/1303/files#diff-e5c96c9122118a911c895ffa5dbd7b00R111) does that?

[jacobperron]

My understanding was that this feature doesn't exist in ROS 1 (yet), so what do we have to be backwards compatible with?

[SteveMacenski]

Ah, yes. Sorry I pointed out this line because of the instructions for [thing]:// not the .srv file in particular. I wanted to make sure that in all cases that the map.yaml file that contains the image: testmap.png field will accept image: testmap.png as well as image: file:///path/to/testmap.png and image: package://my_map_package/maps/testmap.png. That way we're backwards compatible with relative or absolute paths but then now we can use the package path (which will be super nice)

Your question made me realize maybe I'm misintepreting this - this is just for the loading from remote servers of the... yaml file? How does the png/jpeg/pgm get picked up? It reads to me like the read_to_temp only grabs the yaml file, not the image file inside of it too.

I think this PR has gotten to complex for at least me. I think the load map service should be introduced before we start messing around with remote files or changing how we formulate the paths. It seems that if you're going to accept [thing]:// anywhere, it should be in the image: [image-name].[image-extension], or at least doing that as well when you get the yaml file from remote, getting its matched image file too

I think right now this only grabs the remote yaml file and just assumes the image inside the yaml is valid? That's not a good assumption. I don't think we should introduce that feature in this PR, it brings up too many blocking questions. For instance, what does an absolute path now mean when you pulled a file from the server, or relative paths when you're rewriting the path by downloading it. package:// is the only thing that would have that make sense but assumes the same packages installed and the same version of those packages are installed. This is much larger question I don't think there's a canned prepared answer to fix

[mkhansenbot]

Steve, you bring up good points and I think I've addressed them in this PR (which is why it took longer than I hoped).

• if "package", "http" or "file" are not the first words in the path given, resource_retriever is not used, instead it is treated as an absolute local file path (same as before, so we stay compatible with existing files)
• if any of those are given, it will fetch the yaml file, parse it, and then treat the image path as a relative path to the yaml file. So if the yaml is at "http://my.server/file.yaml" and the image name given is "file.png" it will be treated as "http://my.server/file.png".

I didn't test the "package" version, but I did test the "http", "file" and default. I've added unit tests for all of them.

[SteveMacenski]

For bullet 1: only absolute file path? Originally it worked with relative paths and from looking at the code I don’t see in particular why relative would now fail.

For bullet 2: doesn’t that imply it ignores the yaml key for the image name and just assumes it has the same name in the same directory? That’s not right... I also only see where you download the remote yaml, where’s the image retrieval?

[SteveMacenski]

I would disagree, they have to be changable. Just because the yaml file is being retrieved locally or on a server doesn't mean that the image file will be described the same way. Ei, it shouldn't be on the resource itself to know where the caller is (remote, local, absolute, relative). The yaml doesn't have to be anywhere near the image file, or like in your example 2 its gotten from remote but the image key is by a package path and what happens if that package isn't installed on the caller's machine?

Also we need to be caching these maps, we cannot assume that for every time the server is called that its going to have internet access, if its called it should be downloaded and stored locally so that it can be retrieved locally before trying to call out to a remote server. You don't want to be trying to change maps in the middle of a warehouse in a dead zone and then sit there forever because you have no connection

[SteveMacenski]

This has alot of edge cases and problems needing to be thought about more. I think all the webstuff should be stripped out of this PR, its trying to change too much at once. The original ticket was just to have a change map service exposed. It didn't have anything to do with changing how we represent image: yaml keys or retrieving remote resources. That seems to be a whole other conversation

[jacobperron]

Just because the yaml file is being retrieved locally or on a server doesn't mean that the image file will be described the same way

True. I was suggesting we make this assumption for simplicity. As I suggested, we can instead use a URL for the image key, then it doesn't matter where the yaml was retrieved from. There are still edge cases if we also allow regular paths (e.g. do we interpret them as being local or relative to the yaml). Perhaps, removing any assumptions about locality between the yaml and image makes things easier.

in your example 2 its gotten from remote but the image key is by a package path and what happens if that package isn't installed on the caller's machine?

My second example doesn't show the image key is a package path. It is a path relative to map.yaml.

Also we need to be caching these maps, we cannot assume that for every time the server is called that its going to have internet access, if its called it should be downloaded and stored locally so that it can be retrieved locally before trying to call out to a remote server.

This sounds like an additional feature to be added to the map server and beside the question of what the semantics are for the URL and image key.

This has alot of edge cases and problems needing to be thought about more. I think all the webstuff should be stripped out of this PR, its trying to change too much at once

+1

It's good that we're having this discussion! But, we should probably move it to ros/common_msgs#152. I agree that for the first pass, this PR should be an incremental change (e.g. exposing the service). I just wanted to make sure we define the service interface appropriately for the future.

[SteveMacenski]

For the srv type, I have nothing to add, I think its fine and I wouldn't change anything about it. I think the question is in the behavior of use.

This sounds like an additional feature to be added to the map server and beside the question of what the semantics are for the URL and image key.

I don't think it is, I think its a precondition to anything with non-local retrieval. We cannot have a situation where we're dependent on the internet to continuously download the same 5 assets we downloaded yesterday.

I think you and I agree now that this should probably be part of another discussion though, whether its on common msgs or in a ticket here (common_msgs is as good a place as any).

[jacobperron]

For discussion about the behavior, it's probably best to create a ticket in this repo since I think it specific to the nav2_map_server.

[mkhansenbot]

OK, I backed out the resource retriever support since that usage seems to be unclear. Let's get this in for simple local files only, and I'll open a second PR with those changes and we can discuss separately.

[SteveMacenski]

Can we not throw? I don't like servers going down when there's other actions that can be taken to correct.

[mhpanah]

Looks good.