Skip to content

Mypy nav2 simple commander #5059

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

Merged

Conversation

leander-dsouza
Copy link
Contributor


Basic Info

Info Please fill out this column
Ticket(s) this addresses #4916
Primary OS tested on Ubuntu
Robotic platform tested on N/A
Does this PR contain AI generated software? No

Description of testing performed

  • Validated linting using pre-commit.
  • Tested package using colcon.
  • Corrected unsafe fixes using ruff - ruff check --select UP --fix --unsafe-fixes nav2_simple_commander

Description of contribution in a few bullet points

  • Restructured nav2_simple_commander for strict mypy.

Description of documentation updates required from your changes

  • N/A

Description of how this change was tested

  • Tested using pre-commit, colcon testing, and ruff.

Future work that may be required in bullet points

  • N/A

For Maintainers:

  • Check that any new parameters added are updated in docs.nav2.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

@leander-dsouza leander-dsouza marked this pull request as ready for review April 5, 2025 04:07
Copy link

codecov bot commented Apr 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

see 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

Thanks for this, I know this one is probably the most effort. There's a few instances where the server is short and not going to be spinning multiple times (get paths, smooth paths) where the changes would create dead code that I think can be reverted. I know it mimicks the style of other long-running tasks (follow path, navigate to pose) but these don't long-blocking tasks with timeouts on the spin

@leander-dsouza leander-dsouza force-pushed the mypy_nav2_simple_commander branch from b6f8b5a to d982ba6 Compare April 8, 2025 02:49
@leander-dsouza
Copy link
Contributor Author

Thanks for this, I know this one is probably the most effort.

No worries, Steve. I am glad that I could help.

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

I also have this PR open that messes with the simple commander API (#5056). I change a few things like None / TaskEnum for actions so we know what action we should be getting task completion or feedback for.

I also have getRoute(self, start, goal, use_start=False) where start/goal can be either be int or PoseStamped.

Is there a good way around these?

For reference: https://github.com/ros-navigation/navigation2/blob/bc79b6b07f3a8ce89579682265c88cdd8174fcb9/nav2_simple_commander/nav2_simple_commander/robot_navigator.py

@leander-dsouza leander-dsouza force-pushed the mypy_nav2_simple_commander branch from d982ba6 to e2bc291 Compare April 8, 2025 19:26
@leander-dsouza
Copy link
Contributor Author

I also have this PR open that messes with the simple commander API (#5056). I change a few things like None / TaskEnum for actions so we know what action we should be getting task completion or feedback for.

I also have getRoute(self, start, goal, use_start=False) where start/goal can be either be int or PoseStamped.

Is there a good way around these?

  • I think after rebasing this PR, you can first check for conflicting types by:

    ament_mypy --config tools/pyproject.toml nav2_simple_commander/
  • For declaring the type of start/goal, you can use Union:

    from typing import Optional, Union
    from nav_msgs.msg import Path
    from nav2_msgs.msg import Route
    
    def getRoute(self, start: Union[int, PoseStamped],
                 goal: Union[int, PoseStamped], use_start: bool = False) \
            -> Optional[list[Union[Path, Route]]]:
        """Send a `ComputeRoute` action request."""
        self.clearTaskError()
        rtn = self._getRouteImpl(start, goal, use_start=False)
    
        if self.status != GoalStatus.STATUS_SUCCEEDED:
            self.setTaskError(rtn.error_code, rtn.error_msg)
            self.warn('Getting route failed with'
                        f' status code:{self.status}'
                        f' error code:{rtn.error_code}'
                        f' error msg:{rtn.error_msg}')
            return None
    
        return [rtn.path, rtn.route]

@leander-dsouza leander-dsouza force-pushed the mypy_nav2_simple_commander branch from e2bc291 to e2bec79 Compare April 8, 2025 19:42
@SteveMacenski
Copy link
Member

Sweet, thanks!

@SteveMacenski SteveMacenski merged commit d518dd3 into ros-navigation:main Apr 9, 2025
13 checks passed
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.

2 participants