rclpy
rclpy copied to clipboard
ros2
Deal with ParameterUninitializedException for parameter service (backport #1033)
This is an automatic backport of pull request #1033 done by Mergify. Cherry-pick of e442ad2278687a2d5994e580309fe31a435b6421 has failed:
On branch mergify/bp/humble/pr-1033
Your branch is up to date with 'origin/humble'.
You are currently cherry-picking commit e442ad2.
(fix conflicts and run "git cherry-pick --continue")
(use "git cherry-pick --skip" to skip this patch)
(use "git cherry-pick --abort" to cancel the cherry-pick operation)
Changes to be committed:
modified: rclpy/CMakeLists.txt
modified: rclpy/rclpy/parameter_service.py
Unmerged paths:
(use "git add/rm <file>..." as appropriate to mark resolution)
deleted by us: rclpy/test/test_parameter_client.py
To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
Mergify commands and options
More conditions and actions can be found in the documentation.
You can also trigger Mergify actions by commenting on this pull request:
@Mergifyio refreshwill re-evaluate the rules@Mergifyio rebasewill rebase this PR on its base branch@Mergifyio updatewill merge the base branch into this PR@Mergifyio backport <destination>will backport this PR on<destination>branch
Additionally, on Mergify dashboard you can:
- look at your merge queues
- generate the Mergify configuration with the config editor.
Finally, you can contact us on https://mergify.com
![mergify[bot] avatar](https://avatars.githubusercontent.com/in/10562?v=4 "Published by a pub.dev verified publisher"){kind=small}
rclpy change looks good to me, but it adds rclpy/test/test_parameter_client.py file, that i am not sure if we can add the whole file to humble. there could be dependent API to come with this test.
@Barry-Xu-2018 what do you think?
https://github.com/ros2/rclpy/pull/1042 and https://github.com/ros2/rclpy/pull/1043 have the conflict as well.
Yes. I will check it.
After checking, #1041 (Humble) and #1042 (Galactic) can apply fix, but test codes cannot be used since current test code depends on parameter_client. Besides, I find Humble and Galactic only test class Parameter (not test class ParameterService).
Current fixing code is very simple. Is it necessary to re-write new code for Humble and Galactic? @fujitatomoya @ivanpauno
BTW, #1043 (Foxy) should be cancelled. It has different codes.
Current fixing code is very simple. Is it necessary to re-write new code for Humble and Galactic?
i would say let's do that. i think it would be better to fix this issue instead of having the problem in humble and galactic.
BTW, https://github.com/ros2/rclpy/pull/1043 (Foxy) should be cancelled. It has different codes.
probably we can cancel, if anyone else wants this fix into foxy, we could ask for help?
@ivanpauno what do you think?
Is it necessary to re-write new code for Humble and Galactic?
Sorry. My description is unclear. The new code mentioned by me is only for test.
Adding the parameter client tests seem unrelated, I would only backport the fix
https://github.com/ros2/rclpy/issues/1030 could have a regression test that doesn't use the parameter client, as the original bug was unrelated to it. It may be good to have that.
Adding the parameter client tests seem unrelated, I would only backport the fix
Okay.
https://github.com/ros2/rclpy/issues/1030 could have a regression test that doesn't use the parameter client, as the original bug was unrelated to it. It may be good to have that.
Do you mean that I need to re-write test code (without using parameter client) and make a new PR for #1033 (The fix for #1030) ?
Do you mean that I need to re-write test code (without using parameter client)
More than rewriting, I would suggest adding a new test case that doesn't use the parameter client to check for https://github.com/ros2/rclpy/issues/1030 regressions, and add that to rolling.
and make a new PR for https://github.com/ros2/rclpy/pull/1033 (The fix for https://github.com/ros2/rclpy/issues/1030) ?
I think that once that test is ready you can also add it here (or in a new backport if you prefer).
Adding the parameter client tests seem unrelated, I would only backport the fix
Actually, backporting only the parameter client tests shouldn't do any harm. I think that the main issue is that the parameter client itself isn't available in Humble.
@Barry-Xu-2018 @fujitatomoya this can be updated now that https://github.com/ros2/rclpy/pull/1045 was merged.
@fujitatomoya is there another way to move this forward you know of? Perhaps a reviewer with some availability to get this in?
@iuhilnehc-ynos can you review this? i do need another review for this, btw, CI unstable failure is not related to this PR.
CC: @clalancette @audrow @mjcarroll
The warning is fixed in https://github.com/ros2/rclpy/pull/1066, which is merged. Please merge/rebase with the latest humble, so that we can make the CI green.
@iuhilnehc-ynos @Barry-Xu-2018 please start CI once all comments are resolved.
@clalancette @fujitatomoya
Could either of you please help to merge it?
Please use squash and merge and remove the committers unrelated to this PR.
@clalancette
It's a long story that I asked @Barry-Xu-2018 to rebase this branch in https://github.com/ros2/rclpy/pull/1041#issuecomment-1501390215 because there exists warnings in CI and I have no permission to rebase mergify/bp/humble/pr-1033 directly.
Currently, I think you can just squash and merge it. Please notice that
$ git diff origin/humble origin/mergify/bp/humble/pr-1033 --stat
rclpy/CMakeLists.txt | 1 +
rclpy/rclpy/parameter_service.py | 4 ++--
rclpy/test/test_parameter_service.py | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 83 insertions(+), 2 deletions(-)
CI:
- Linux
- Linux-aarch64
- Windows
once CI goes green, it is ready to merge.
@clalancette all of your previous comments are addressed, i will go ahead to merge this.