Skip to content

Add graph download/upload/clear methods#30

Merged
amessing-bdai merged 12 commits intorai-opensource:mainfrom
sktometometo:PR/add-graph-manipulation-methods
Jun 29, 2023
Merged

Add graph download/upload/clear methods#30
amessing-bdai merged 12 commits intorai-opensource:mainfrom
sktometometo:PR/add-graph-manipulation-methods

Conversation

@sktometometo
Copy link
Copy Markdown
Contributor

@sktometometo sktometometo commented Jun 24, 2023

  • Add graph downloading methods
  • Add public methods for clearing, uploading and downloading graph methods

Checked with a robot like

from spot_wrapper.wrapper import SpotWrapper
import logging

logging.basicConfig(level=logging.INFO)
logger = logging.getLogger()

wrapper = SpotWrapper(username='hoge', password='fuga', hostname='192.168.50.3', robot_name='myspot', logger=logger)
wrapper.claim()

wrapper.upload_graph('path to graph')
wrapper.list_graph(None)
wrapper.download_graph('directory for downloading')
wrapper.clear_graph()
wrapper.list_graph(None)

Errors will show up during list_graph method. This will be fixed with #31

@sktometometo sktometometo changed the title [WIP] Add graph manipulation methods [WIP] Add graph manipulation methods (dependent to https://github.com/bdaiinstitute/spot_wrapper/pull/28) Jun 24, 2023
@sktometometo sktometometo changed the title [WIP] Add graph manipulation methods (dependent to https://github.com/bdaiinstitute/spot_wrapper/pull/28) [WIP] Add graph manipulation methods (dependent to #28) Jun 24, 2023
@sktometometo sktometometo force-pushed the PR/add-graph-manipulation-methods branch from d1bdfeb to 96cdeef Compare June 24, 2023 11:59
@sktometometo sktometometo changed the title [WIP] Add graph manipulation methods (dependent to #28) Add graph manipulation methods Jun 24, 2023
@sktometometo sktometometo force-pushed the PR/add-graph-manipulation-methods branch from 820471c to 43cac99 Compare June 24, 2023 12:17
@sktometometo sktometometo marked this pull request as ready for review June 24, 2023 12:18
@sktometometo sktometometo mentioned this pull request Jun 24, 2023
@sktometometo
Copy link
Copy Markdown
Contributor Author

This feature is originally introduced in this old PR for spot_ros. I am using this in our lab's fork. But I would like to merge this feature to upstream repo.

Who should I ask for a review about it?

@sktometometo sktometometo changed the title Add graph manipulation methods Add graph download/upload/clear methods Jun 28, 2023
Comment thread spot_wrapper/wrapper.py Outdated
self._clear_graph()
return True, "Success"
except Exception as e:
return False, f"Error: {e}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More descriptive error message?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the error message.

Comment thread spot_wrapper/wrapper.py Outdated
Args:
upload_path (str): Path to the directory of the map."""
try:
self._clear_graph()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The upload graph function should strictly serve to upload the graph without implicitly erasing any existing data. This ensures the process is atomic, meaning it doesn't interfere with the current state of the graph.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I have removed implicit _clear_graph() from here.

Comment thread spot_wrapper/wrapper.py Outdated
)
return success, message
except Exception as e:
return False, f"Error: {e}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More descriptive error message?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the error message.

Comment thread spot_wrapper/wrapper.py Outdated
"""Download graph and snapshots from robot.

Args:
download_path (str): Directory where graph and snapshotw are downloaded from robot.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix typo.

Comment thread spot_wrapper/wrapper.py Outdated
"""Write data to a file.

Args:
filepath (str) : Path of file wher data will be written.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix typo.

Comment thread spot_wrapper/wrapper.py Outdated
]

def clear_graph(self) -> typing.Tuple[bool, str]:
"""Clear a graph in a robot.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be clearer, e.g. the docstring for _clear_graph is Clear the state of the map on the robot, removing all waypoints and edges.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the method docstring. Also added that it removes snapshots only in the ram.

Comment thread spot_wrapper/wrapper.py Outdated
return False, f"Error: {e}"

def upload_graph(self, upload_path: str) -> typing.Tuple[bool, str]:
"""Upload graph and snapshots to robot.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, could be more descriptive since this is meant to be used externally.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the docstring.

Comment thread spot_wrapper/wrapper.py
os.path.join(download_path, "edge_snapshots", edge.snapshot_id),
edge_snapshot.SerializeToString(),
)
return True, "Success"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking, is there a SpotSDK example reference for this functionality?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sktometometo
Copy link
Copy Markdown
Contributor Author

Thank you for review, I will update the branch.

@sktometometo sktometometo marked this pull request as draft June 29, 2023 00:18
@sktometometo
Copy link
Copy Markdown
Contributor Author

Checked with our spot.

@sktometometo sktometometo marked this pull request as ready for review June 29, 2023 01:35
@kzheng-bdai
Copy link
Copy Markdown
Contributor

I tested this out (by running basically the code in description).

I do get an error at list graph regarding logging (as expected). Uploading and downloading appears to work.

The error:

Call stack:
  File "/workspaces/bdai/projects/_experimental/adhoc_tests/test_spot_wrapper_pr_30.py", line 17, in <module>
    success, msg = wrapper.list_graph(None)
  File "/workspaces/bdai/ws/build/spot_wrapper/spot_wrapper/wrapper.py", line 1535, in list_graph
    ids, eds = self._list_graph_waypoint_and_edge_ids()
  File "/workspaces/bdai/ws/build/spot_wrapper/spot_wrapper/wrapper.py", line 2203, in _list_graph_waypoint_and_edge_ids
    ) = graph_nav_util.update_waypoints_and_edges(
  File "/workspaces/bdai/ws/build/spot_wrapper/spot_wrapper/graph_nav_util.py", line 123, in update_waypoints_and_edges
    logger.info(
Message: '(Edge) from waypoint id: '
Arguments: ('acting-conger-SeSRo1s3Mff94lesVJNnuw==', ' and to waypoint id: ', 'agone-hound-N2eQdZqi3hRzWqeSXIQN6g==')

Commenting out list_graph I get this:

INFO:root:Initialising robot at <ip>
INFO:root:Trying to authenticate with robot...
INFO:root:Successfully authenticated.
INFO:root:Creating clients...
INFO:root:No velodyne point cloud service is available.
INFO:root:Starting lease check-in
INFO:ros_spot:Starting estop check-in
INFO:root:Loading the graph from disk into local storage...
INFO:root:Loaded graph has 50 waypoints and 62 edges
INFO:root:Uploading the graph and snapshots to the robot...
INFO:root:Uploaded snapshot_tippy-setter-ribOp8tCzjBOE8ICO0DISA==
INFO:root:Uploaded snapshot_dewy-colt-IEHfxdFe8x.cDjw7nK6e6Q==
INFO:root:Uploaded snapshot_plane-remora-WFRFjvOzeWoyC9K+PuRg+w==
INFO:root:Uploaded snapshot_six-dugong-Mt.hM6+btIIhkOjJyWsQQw==
INFO:root:Uploaded snapshot_pupal-kelp-3uW+xliiZipj4g34P+rk1Q==
INFO:root:Uploaded snapshot_frizzy-gander-DJ7EpEYzjDPP58YZGCBbww==
INFO:root:Uploaded snapshot_carved-ant-iUwmERlEv.mWzbsdoI9o0w==
INFO:root:Uploaded snapshot_beaded-antler-9kCXxAAshwVRidHGikgJ2g==
INFO:root:Uploaded snapshot_blurry-fish-nJ7XK48YxsKkG6chG1Zmlw==
INFO:root:Uploaded snapshot_gowned-finch-.MJyBFGaLqNxS5z+xgpCUA==
INFO:root:Uploaded snapshot_astute-fox-mxCRFW4IxofC4sLn.ERD5A==
INFO:root:Uploaded snapshot_hulky-gerbil-Nr.rocs1Nbyya6QG5FcoTw==
INFO:root:Uploaded snapshot_sacked-cocoon-.gcajyeFG4VKjPVlZmodiA==
INFO:root:Uploaded snapshot_upland-beaver-ssYvM34wCEAT0qo7s1lxCQ==
INFO:root:Uploaded snapshot_dimmed-horse-Uq3lbol14xTwDB0Jhus1XQ==
INFO:root:Uploaded snapshot_iffy-vervet-2iOgg7ErP2NxHRpKrsaSWg==
INFO:root:Uploaded snapshot_amuck-tern-nh6F9ca9C9gGIhzAysSLVQ==
INFO:root:Uploaded snapshot_droopy-conger-pSbzIMMrQGzrNZ0gJMPHyw==
INFO:root:Uploaded snapshot_fifty-botfly-U2Q4tfg.9+Q0BLW1+OQnYA==
...
INFO:root:Uploaded edge_snapshot_id_fifth-trout-.iSy61cC5p+3aKE2Eu7Hyw==
INFO:root:Uploaded edge_snapshot_id_sneezy-gadfly-R+SWYaNEnVnOW3fuoG1zBg==
INFO:root:Uploaded edge_snapshot_id_corny-ibex-+PXYrGSWUvW9EZOUaoaQ3w==
INFO:root:Uploaded edge_snapshot_id_woozy-falcon-PHO5Rgi952vYg6XdsnB8Tg==
INFO:root:Uploaded edge_snapshot_id_sallow-wren-nSbIOBXhD8LeB2nXmajr5g==
INFO:root:Uploaded edge_snapshot_id_tagged-snipe-ev3WUzBJ2wNDtGLUtx+jHg==
...
INFO:root:Upload complete! The robot is currently not localized to the map; please localize the robot
WARNING:root:Failed to download edge snapshot:
WARNING:root:Failed to download edge snapshot:
WARNING:root:Failed to download edge snapshot:
WARNING:root:Failed to download edge snapshot:
WARNING:root:Failed to download edge snapshot:
WARNING:root:Failed to download edge snapshot:
WARNING:root:Failed to download edge snapshot:
WARNING:root:Failed to download edge snapshot:
WARNING:root:Failed to download edge snapshot:
WARNING:root:Failed to download edge snapshot:
WARNING:root:Failed to download edge snapshot:
WARNING:root:Failed to download edge snapshot:
WARNING:root:Failed to download edge snapshot:
INFO:root:0 waypoints:

Copy link
Copy Markdown
Contributor

@kzheng-bdai kzheng-bdai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested after fixing the logging error and looks good.
Also tested our own GraphNav functionalities.

@amessing-bdai amessing-bdai merged commit 362c98d into rai-opensource:main Jun 29, 2023
@sktometometo sktometometo deleted the PR/add-graph-manipulation-methods branch June 29, 2023 23:51
@sktometometo
Copy link
Copy Markdown
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants