Add verbose info for topic list #351

evshary · 2019-10-06T09:57:56Z

According to #347, the PR adds option -v to show more info for topic list.

claireyywang

Happy Hacktoberfest and thanks for submitting this PR! I would recommend using a function to handle the string formatting part, since publisher and subscriber use very similar lines to structure the print statement. The following are a few nitpicks.

claireyywang · 2019-10-08T18:41:37Z

ros2topic/ros2topic/verb/list.py

+                    if pub_cnt < 1:
+                        continue


I would suggest if pub_cnt:

claireyywang · 2019-10-08T18:42:10Z

ros2topic/ros2topic/verb/list.py

+                    if sub_cnt < 1:
+                        continue


same as above, if sub_cnt:

claireyywang · 2019-10-08T18:43:02Z

ros2topic/ros2topic/verb/list.py

+                    cnt_str = str(pub_cnt) + ' publisher' + ('s' if pub_cnt > 1 else '')
+                    msg = ' * {topic_name} [{topic_types_formatted}] {cnt_str}'
+                    print(msg.format_map(locals()))
+                print('')


You can probably add new line to the string format than use print('')

claireyywang · 2019-10-08T18:43:13Z

ros2topic/ros2topic/verb/list.py

+                    cnt_str = str(sub_cnt) + ' subscriber' + ('s' if sub_cnt > 1 else '')
+                    msg = ' * {topic_name} [{topic_types_formatted}] {cnt_str}'
+                    print(msg.format_map(locals()))
+                print('')


same as above

claireyywang

Thanks for following up! Looks good to me with green CI. I will run CI for you once a few nitpicks are adjusted.

ros2topic/ros2topic/verb/list.py

evshary · 2019-10-14T02:13:00Z

@claireyywang Thanks for your suggestions. I think it's much more better.

dirk-thomas · 2019-10-14T02:27:56Z

ros2topic/ros2topic/verb/list.py


    def main(self, *, args):
-        with NodeStrategy(args) as node:
+        topic_info = []
+        with DirectNode(args) as node:


Using aDirectNode here instead of a NodeStrategy will not use the darlin and therefore result in either a longer time to wait for discovery or incomplete information. So please using the NodeStrategy here.

I use NodeStrategy instead now. However, I need to register count_publishers and count_subscribers to use these functions, so I also modified the daemon.

found error introduced by new change

claireyywang · 2019-10-21T23:06:08Z

@evshary Can you confirm that it works? The new change is giving me error.

Traceback (most recent call last):
  File "/home/claire/ros2_ws/install/ros2cli/bin/ros2", line 11, in <module>
    load_entry_point('ros2cli==0.8.2', 'console_scripts', 'ros2')()
  File "/home/claire/ros2_ws/install/ros2cli/lib/python3.6/site-packages/ros2cli/cli.py", line 69, in main
    rc = extension.main(parser=parser, args=args)
  File "/home/claire/ros2_ws/install/ros2topic/lib/python3.6/site-packages/ros2topic/command/topic.py", line 43, in main
    return extension.main(args=args)
  File "/home/claire/ros2_ws/install/ros2topic/lib/python3.6/site-packages/ros2topic/verb/list.py", line 59, in main
    pub_cnt = node.count_publishers(topic_name)
  File "/usr/lib/python3.6/xmlrpc/client.py", line 1112, in __call__
    return self.__send(self.__name, args)
  File "/usr/lib/python3.6/xmlrpc/client.py", line 1452, in __request
    verbose=self.__verbose
  File "/usr/lib/python3.6/xmlrpc/client.py", line 1154, in request
    return self.single_request(host, handler, request_body, verbose)
  File "/usr/lib/python3.6/xmlrpc/client.py", line 1170, in single_request
    return self.parse_response(resp)
  File "/usr/lib/python3.6/xmlrpc/client.py", line 1342, in parse_response
    return u.close()
  File "/usr/lib/python3.6/xmlrpc/client.py", line 656, in close
    raise Fault(**self._stack[0])
xmlrpc.client.Fault: <Fault 1: '<class \'Exception\'>:method "count_publishers" is not supported'>

evshary · 2019-10-22T04:38:14Z

@claireyywang You may need to rebuild ros2cli and restart the daemon again.

colcon build --packages-select roscli
ros2 daemon stop
ros2 daemon start

claireyywang · 2019-10-25T22:00:56Z

@claireyywang You may need to rebuild ros2cli and restart the daemon again.
colcon build --packages-select roscli
ros2 daemon stop
ros2 daemon start

I think that might be a behaviour caused by outdated packages. We recently merged a fix into master that resolved the daemon issue. I have rebased your forked branch with our latest master branch, and will run CI for you. If everything comes back green then it should be good to merge.

claireyywang · 2019-10-28T19:16:36Z

Linux
Linux-aarch64
OSX
Windows

nuclearsandwich · 2019-10-28T21:39:30Z

I've re-triggered the Windows CI job as the one above had stalled

claireyywang · 2019-11-11T17:19:31Z

Update: process seems to be hanging on windows CI machines. Investigating...

nuclearsandwich · 2019-11-11T20:31:18Z

The hang is likely related to ros2/build_farmer#248. The hang has so far not been 100% reproducible so re-running CI is also an option.

claireyywang · 2019-11-11T20:54:49Z

I've been running windows CI every day last week without explicitly posting here as part of investigation, but the process always hung. Here's another rerun

nuclearsandwich · 2019-11-11T21:50:00Z

I've been running windows CI every day last week without explicitly posting here as part of investigation, but the process always hung. Here's another rerun

While this is a bummer for this PR. I'm a little excited because this might increase the likelihood of the hang in my attempts to reproduce. I've triggered which tests this PR against ros2/rclpy#456.

evshary · 2019-12-15T08:49:32Z

Since the PR seems to conflict with #420, I've rebased the branch to latest master.

claireyywang · 2019-12-30T18:45:43Z

here we go again
Linux
Linux-aarch64
OSX
Windows

claireyywang · 2020-01-02T22:14:11Z

@nuclearsandwich Are all the SIGINTs in windows CI supposed to terminate the process hang before?

Signed-off-by: evshary <[email protected]>

Co-Authored-By: Claire Wang <[email protected]> Signed-off-by: evshary <[email protected]>

We also need to register functions for NodeStrategy: count_publishers and count_subscribers. Signed-off-by: evshary <[email protected]>

audrow · 2021-03-12T17:11:24Z

I rebased and tested on my machine - here's another run of CI:

Linux
Linux-aarch64
macOS
Windows

audrow

@evshary, this is a nice feature - thanks for the PR! Could you

fix the linter warnings, found in CI,
and add some tests for this feature? You can see similar tests around here.

Let me know if you need any help with these, and feel free to re-request a review from me once you've made these changes.

Signed-off-by: ChenYing Kuo <[email protected]>

evshary · 2021-03-19T12:44:24Z

@audrow I guess I need some help on adding tests to this feature.
It seems like I need to parse the output of ros2 topic list -v and check whether it's correct or not.
However, I'm not really sure how to run the test and see what the output should be.
The only way I know to run the test is colcon test --packages-select ros2topic, but it takes time and I can't see the specific test output.
Could you give me some suggestions about how to write and run the test more simply?

audrow · 2021-03-19T15:44:19Z

I'm not sure of the best way to run the tests.

One thing that I've been doing lately is to create a Python file (can be outside of the package) that imports what is needed and runs what you want to test so that you can inspect it (this doesn't necessarily need tests, I often just use functions with assert statements). I've been doing this after sourcing my ROS 2 workspace. Then once it seems to be working, I move the code into the package's test files and run them with Colcon, as you're doing now.

Hopefully this helps.

clalancette · 2021-03-19T15:52:20Z

The only way I know to run the test is colcon test --packages-select ros2topic, but it takes time and I can't see the specific test output.
Could you give me some suggestions about how to write and run the test more simply?

The tests are run through pytest, and colcon supports --pytest-args. So you should be able to do something like:

colcon test --event-handlers console_direct+ --packages-select ros2topic --pytest-args -k copyright

Which will only run the copyright test.

Signed-off-by: ChenYing Kuo <[email protected]>

evshary · 2021-03-20T02:58:32Z

colcon test --event-handlers console_direct+ --packages-select ros2topic --pytest-args -k copyright

Which will only run the copyright test.

It works like a charm! Thank you @clalancette and @audrow .
@audrow I've added the test item now. Please help me to check whether it's ok or not.

fujitatomoya

lgtm with minor comment.

ros2topic/ros2topic/verb/list.py

fujitatomoya · 2021-03-21T07:14:56Z

CI:

Linux
Linux-aarch64
macOS
Windows

fujitatomoya · 2021-03-22T00:03:17Z

@audrow @clalancette CI green, i think it is okay to go in. could you do me a favor?

audrow

The test looks good. Now that I'm looking more closely at the code, I think it would be good to do some restructuring.

One possibility would be do something like the following:

for (topic_name, topic_types) in topic_names_and_types:
    msg = f'{topic_name}'
    topic_types_formatted = ', '.join(topic_types)  # I think this can be moved into the next if statement
    if args.show_types or args.verbose:
        msg += f' [{topic_types_formatted}]'
    if args.show_publishers or args.verbose:
        msg += get_topic_publishers_list(topic_types)
    if args.show_publishers or args.verbose:
        msg += get_topic_subscriptions_list(topic_types)
    print(msg)

Where get_topic_publishers_list and get_topic_subscriptions_list are quite similar to your show_topic_info (maybe both using a common function).

What do you think @fujitatomoya?

ros2topic/ros2topic/verb/list.py

audrow · 2021-03-22T02:18:14Z

ros2topic/ros2topic/verb/list.py

@@ -18,6 +18,18 @@
 from ros2topic.verb import VerbExtension


+def show_topic_info(topic_info, isPub):


A few nitpicks

isPub isn't really Pythonic. Could you do is_publisher instead? Although, maybe there's a better way to structure this as "subscription" isn't really the opposite of "publisher", as anything else would not be a publisher.

I prefer longer names, as I find it much more readable. For example, rather than using cnt, you can just write count.

I think this function should go to the bottom of the file, rather than be at the top. You could also make it a static method of the ListVerb class.

I agree with you.

ros2topic/ros2topic/verb/list.py

audrow · 2021-03-22T04:06:20Z

ros2topic/ros2topic/verb/list.py

+            for (topic_name, topic_types) in topic_names_and_types:
+                pub_cnt = node.count_publishers(topic_name)
+                sub_cnt = node.count_subscribers(topic_name)
+                topic_info.append((topic_name, topic_types, pub_cnt, sub_cnt))


To me, having a list of tuples with four elements isn't desirable. In topic_names_and_types, there are two and they're right there in the variable name. Maybe there is a better way to do this. Perhaps, just compute them as you need them in the show_topic_info function.

Why I compute them here is that I need the NodeStrategy. I'm not sure how to pass NodeStrategy to show_topic_info is a better way.

def show_topic_info(args, topic_info, isPub): with NodeStrategy(args) as node: ....

@audrow I add the if-condition to avoid running count_publishers and count_subscribers while there is no verbose. I'm not sure whether it's a better way, but I try to avoid pass args into another function just for with NodeStrategy(args) as node.
What do you think about it?

Sounds good.

ros2topic/ros2topic/verb/list.py

audrow · 2021-03-22T04:19:53Z

ros2topic/ros2topic/verb/list.py

+        parser.add_argument(
+            '-v', '--verbose', action='store_true',
+            help='list full details about each topic')


Perhaps the command should change to something like --show-connections (maybe there's a better name) or be broken into --show-subscriptions and --show-publishers. You could also have a --verbose command that enables everything, but it would be nice to have the ability to enable things individually.

I prefer to add this feature in another PR.

audrow · 2021-03-22T04:41:10Z

@evshary, I wrote comments as I was reviewing things before coming to the opinion that it would be good to do some restructuring. They should be good anyways to see the kinds of things I'm looking for, e.g., consistent styling. Let me know if you have any questions or need any help.

Signed-off-by: ChenYing Kuo <[email protected]>

audrow · 2021-03-26T17:35:46Z

@evshary, is this ready for another review?

evshary · 2021-03-27T00:58:17Z

@evshary, is this ready for another review?

Sure. Please let me know if there's anything which can be improved.

audrow

Looks okay to me, I'll rerun CI.

audrow · 2021-03-30T23:39:44Z

Linux
Linux-aarch64
macOS
Windows

audrow · 2021-04-01T19:42:41Z

Windows CI warning is from Cyclone. Merging this.

audrow · 2021-04-01T19:43:24Z

Thanks for the PR, @evshary!

claireyywang suggested changes Oct 8, 2019

View reviewed changes

evshary force-pushed the topic_list_verbose branch from 6865078 to 6d7aa20 Compare October 9, 2019 04:40

evshary requested a review from claireyywang October 10, 2019 13:18

claireyywang previously approved these changes Oct 10, 2019

View reviewed changes

ros2topic/ros2topic/verb/list.py Outdated Show resolved Hide resolved

ros2topic/ros2topic/verb/list.py Outdated Show resolved Hide resolved

evshary force-pushed the topic_list_verbose branch from 5deab56 to 2f83aeb Compare October 14, 2019 02:08

dirk-thomas reviewed Oct 14, 2019

View reviewed changes

dirk-thomas added enhancement New feature or request in progress Actively being worked on (Kanban column) requires-changes labels Oct 14, 2019

evshary force-pushed the topic_list_verbose branch 2 times, most recently from 3f0d386 to 586eb0d Compare October 20, 2019 07:52

evshary requested a review from dirk-thomas October 21, 2019 10:02

claireyywang self-assigned this Oct 25, 2019

claireyywang force-pushed the topic_list_verbose branch from 586eb0d to 2c848ef Compare October 25, 2019 21:58

claireyywang approved these changes Oct 25, 2019

View reviewed changes

evshary force-pushed the topic_list_verbose branch from 2c848ef to 22d5a3c Compare December 15, 2019 08:45

evshary and others added 4 commits March 12, 2021 08:25

Refactor the code and use function instead.

0b14c84

Signed-off-by: evshary <[email protected]>

Use underscore to replace unused variables

d32b728

Co-Authored-By: Claire Wang <[email protected]> Signed-off-by: evshary <[email protected]>

Use the same indentation for consistency

5682647

Co-Authored-By: Claire Wang <[email protected]> Signed-off-by: evshary <[email protected]>

Use NodeStrategy instead of DirectNode for showing topic list.

b61a605

We also need to register functions for NodeStrategy: count_publishers and count_subscribers. Signed-off-by: evshary <[email protected]>

audrow force-pushed the topic_list_verbose branch from 30987f4 to b61a605 Compare March 12, 2021 16:55

audrow requested changes Mar 12, 2021

View reviewed changes

Remove blank line.

a4c438b

Signed-off-by: ChenYing Kuo <[email protected]>

evshary force-pushed the topic_list_verbose branch from a652693 to a4c438b Compare March 19, 2021 12:37

Add test for topic list verbose.

8f0e5a3

Signed-off-by: ChenYing Kuo <[email protected]>

evshary requested a review from audrow March 20, 2021 02:58

fujitatomoya approved these changes Mar 21, 2021

View reviewed changes

ros2topic/ros2topic/verb/list.py Outdated Show resolved Hide resolved

audrow requested changes Mar 22, 2021

View reviewed changes

evshary added 2 commits March 23, 2021 19:01

Restruct the code.

8d176cb

Signed-off-by: ChenYing Kuo <[email protected]>

Restruct the code.

5713a51

Signed-off-by: ChenYing Kuo <[email protected]>

evshary requested a review from audrow March 27, 2021 00:58

audrow approved these changes Mar 30, 2021

View reviewed changes

audrow merged commit d03ee6c into ros2:master Apr 1, 2021

		@@ -18,6 +18,18 @@
		from ros2topic.verb import VerbExtension


		def show_topic_info(topic_info, isPub):

Add verbose info for topic list #351

Add verbose info for topic list #351

Conversation

evshary commented Oct 6, 2019

claireyywang left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claireyywang left a comment

Choose a reason for hiding this comment

evshary commented Oct 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claireyywang commented Oct 21, 2019 • edited Loading

evshary commented Oct 22, 2019

claireyywang commented Oct 25, 2019

claireyywang commented Oct 28, 2019 • edited Loading

nuclearsandwich commented Oct 28, 2019

claireyywang commented Nov 11, 2019

nuclearsandwich commented Nov 11, 2019

claireyywang commented Nov 11, 2019

nuclearsandwich commented Nov 11, 2019

evshary commented Dec 15, 2019

claireyywang commented Dec 30, 2019 • edited Loading

claireyywang commented Jan 2, 2020

audrow commented Mar 12, 2021

audrow left a comment

Choose a reason for hiding this comment

evshary commented Mar 19, 2021

audrow commented Mar 19, 2021

clalancette commented Mar 19, 2021

evshary commented Mar 20, 2021

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya commented Mar 21, 2021

fujitatomoya commented Mar 22, 2021

audrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

audrow commented Mar 22, 2021

audrow commented Mar 26, 2021

evshary commented Mar 27, 2021

audrow left a comment

Choose a reason for hiding this comment

audrow commented Mar 30, 2021 • edited Loading

audrow commented Apr 1, 2021

audrow commented Apr 1, 2021

claireyywang left a comment •

edited

Loading

claireyywang commented Oct 21, 2019 •

edited

Loading

claireyywang commented Oct 28, 2019 •

edited

Loading

claireyywang commented Dec 30, 2019 •

edited

Loading

audrow commented Mar 30, 2021 •

edited

Loading