Skip to content

Prep for kilted #1060

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

Closed
wants to merge 30 commits into from
Closed

Prep for kilted #1060

wants to merge 30 commits into from

Conversation

aaronchongth
Copy link
Member

@aaronchongth aaronchongth commented Jun 24, 2025

What's new

  • image building for jazzy has additional step to build and install orphaned alert messages
  • api server conditionally imports the alert related messages from the orphaned rmf_alert_msgs if using jazzy, otherwise the usual rmf_task_msgs
  • more reusable image build for kilted and rolling
  • retarget latest as rolling
  • updated docs

Self-checks

  • I'm familiar with and follow this Typescript guideline
  • I added unit-tests for new components
  • I tried testing edge cases
  • I tested the behavior of the components that interact with the backend, with an e2e test

Copy link

codecov bot commented Jun 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.10%. Comparing base (ea43420) to head (d3327d6).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1060   +/-   ##
=======================================
  Coverage   70.09%   70.10%           
=======================================
  Files         288      288           
  Lines       14175    14175           
  Branches     1136     1137    +1     
=======================================
+ Hits         9936     9937    +1     
+ Misses       4215     4214    -1     
  Partials       24       24           
Flag Coverage Δ
api-server 79.83% <ø> (+0.03%) ⬆️
rmf-dashboard-framework 67.41% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aaronchongth aaronchongth requested a review from koonpeng June 24, 2025 16:52
@@ -143,6 +142,8 @@ ros2 launch rmf_demos_gz office.launch.xml server_uri:="http://localhost:8000/_i

# Troubleshooting

- Alert related messages were introduced after Open-RMF Jazzy releases, hence it is not available with Open-RMF Jazzy binaries, and have to be built from source to be used by the API server. See https://github.com/open-rmf/rmf-web/issues/1041 for more information, as well as how the [Jazzy API server base image is built](.github/minimal-rmf-jazzy/Dockerfile). ROS 2 distributions after Jazzy can just use the released binaries.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This appears to be the reason why jazzy has it's own dockerfile. This indicates that the api-server is not compatible with jazzy anymore and building with a newer distro is not a solution.

The proper solution is to support both jazzy and the new Alert messages by using conditional imports.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the migration to Jazzy it has already not been compatible in the sense of debian packages, but the API server has never been compatible, we are already using pip to install dependencies on a virtual environment. Since our past decision about releasing the API server (or the lack of ability) onto the buildfarm or pypi, we will be relying on docker images instead, hence rmf_deployment_template

The alerts are used for implementing crucial features of the API server and dashboard, I'd rather keep the feature alive with this caveat instead of rolling a feature back. If a user intends to deploy the API server, only an underlay image is required. At the end of the day, the requirement of using released binaries on any given ROS 2 distro, is to enable release on the buildfarm, which we will not be achieving any time soon without a total re-write.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem is not that we are using packages outside of the distro (pip), it is that we are using non-jazzy rmf packages to interface with jazzy rmf. If there is a breaking change in a rmf message, then there is no longer a commit that we can build off of.

We don't need to remove any of the alert features, we just need to detect if we are running under jazzy or newer and conditionally enable it. Or alternatively, remove the Alert api and branch off on a jazzy branch, but then we will need to maintain both branches so I think a conditional import is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

conditional imports have been added

Signed-off-by: Aaron Chong <[email protected]>
Signed-off-by: Aaron Chong <[email protected]>
@aaronchongth aaronchongth requested a review from koonpeng June 25, 2025 05:33
@@ -143,6 +142,8 @@ ros2 launch rmf_demos_gz office.launch.xml server_uri:="http://localhost:8000/_i

# Troubleshooting

- Alert related messages were introduced after Open-RMF Jazzy releases, hence it is not available with Open-RMF Jazzy binaries, and have to be built from source to be used by the API server. See https://github.com/open-rmf/rmf-web/issues/1041 for more information, as well as how the [Jazzy API server base image is built](.github/minimal-rmf-jazzy/Dockerfile). ROS 2 distributions after Jazzy can just use the released binaries.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem is not that we are using packages outside of the distro (pip), it is that we are using non-jazzy rmf packages to interface with jazzy rmf. If there is a breaking change in a rmf message, then there is no longer a commit that we can build off of.

We don't need to remove any of the alert features, we just need to detect if we are running under jazzy or newer and conditionally enable it. Or alternatively, remove the Alert api and branch off on a jazzy branch, but then we will need to maintain both branches so I think a conditional import is better.

Signed-off-by: Aaron Chong <[email protected]>
Signed-off-by: Aaron Chong <[email protected]>
Signed-off-by: Aaron Chong <[email protected]>
Signed-off-by: Aaron Chong <[email protected]>
Signed-off-by: Aaron Chong <[email protected]>
Signed-off-by: Aaron Chong <[email protected]>
Signed-off-by: Aaron Chong <[email protected]>
@mxgrey mxgrey moved this from Inbox to In Progress in PMC Board Jul 1, 2025
Signed-off-by: Aaron Chong <[email protected]>
Signed-off-by: Aaron Chong <[email protected]>
Signed-off-by: Aaron Chong <[email protected]>
Signed-off-by: Aaron Chong <[email protected]>
Signed-off-by: Aaron Chong <[email protected]>
@aaronchongth aaronchongth requested a review from koonpeng July 7, 2025 08:51
Comment on lines +10 to +19
RUN if [ "$ROS_DISTRO" = "jazzy" ]; then \
apt install -y ros-dev-tools \
&& mkdir -p /rmf && cd /rmf \
&& curl -sL https://github.com/open-rmf/rmf_internal_msgs/archive/jazzy-alert-msgs.tar.gz -o jazzy-alert-msgs.tar.gz \
&& mkdir -p /rmf/src/alert_msgs && tar zxf jazzy-alert-msgs.tar.gz -C /rmf/src/alert_msgs --strip-components=1 && rm jazzy-alert-msgs.tar.gz \
&& cd /rmf \
&& . /opt/ros/$ROS_DISTRO/setup.sh \
&& colcon build --merge-install --install-base /opt/ros/$ROS_DISTRO --cmake-args -DCMAKE_BUILD_TYPE=Release \
&& rm -rf /rmf; \
fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

These alert messages does not seem to be part of rmf-jazzy release, so we cannot use them in a jazzy release.

# isort: off
# pylint: disable=import-error,no-name-in-module
if os.getenv("ROS_DISTRO") == "jazzy":
from rmf_alert_msgs.msg import Alert as RmfAlert, AlertResponse as RmfAlertResponse # pyright: ignore[reportMissingImports,reportAttributeAccessIssue]
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the jazzy branch, we cannot import messages not part of jazzy release. What I would recommend is to refactor out all the alert subscriptions to another file and create a jazzy version of that file that does not do anything. So we would have

alert_gateway.py
alert_gateway_stub.py

alert_gateway_stub.py would just be a stub that does not do anything. Then on the main gateway, do a conditional import for either one of them depending on the env. For pyright to work, we may also need to add a if TYPE_CHECKING: conditional import to tell it to use the stub. pyright may still complain errors in the jazzy CI so we may need to exclude the file as well (only in jazzy CI).

@aaronchongth
Copy link
Member Author

Closing with #1066

@mxgrey mxgrey removed this from PMC Board Aug 12, 2025
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