Make CLI more robust to discovery latency. by hidmic · Pull Request #494 · ros2/ros2cli

hidmic · 2020-04-22T16:31:06Z

Split from #421. This pull request builds on top of #493 and ensures the daemon gets used throughout all CLI tools. It also updates some tests which showed to be flakey nonetheless (with and without this patch).

hidmic · 2020-04-27T15:50:58Z

With fac0a49, this fixes #488.

hidmic · 2020-04-27T18:54:31Z

CI up to ros2topic, ros2service, ros2action, ros2node and ros2component:

Linux
Linux-aarch64
OSX
Windows

dirk-thomas · 2020-04-27T19:07:32Z

With fac0a49, #488 should be now addressed.

Please use a format which will ensure the referenced ticket is automatically closed when the PR is merged.

dirk-thomas · 2020-04-27T19:02:12Z

    )
    try:
-        list_nodes_client.wait_for_service()
+        if not list_nodes_client.wait_for_service(timeout_sec=5.0):


When used in a loop (as in component list) a blocking timeout of 5s can add up to a very long time for the user call.

Fair enough. Timeout is a somewhat arbitrary, larger than zero quantity. What about 2.0 seconds?

I don't mind the value. For this PR you can choose whatever.

The problem is that with the current API all waits happen sequentially. E.g. if the service of N nodes isn't available the CLI will hang for N * timeout. I would prefer that the CLI only blocks for timeout seconds (whatever that value is). But that is pretty much our of scope for this PR.

Please create a follow up ticket for this.

Oh, good point. See #507.

dirk-thomas · 2020-04-27T19:05:53Z

    qos_profile = qos_profile_from_short_keys(
        args.qos_profile, reliability=args.qos_reliability, durability=args.qos_durability)
+    if args.message_type is None:
+        with NodeStrategy(args) as node:


This potentially creates a second direct node (beside the one created a few lines below) which doesn't seem like a good idea.

Same pattern below in multiple cases (even though they preexisted before).

That is true, you might end up creating node, destroying it and creating another one. We do want to use the daemon if available though.

A solution could be to extend NodeStrategy's API to ensure you're dealing with a direct node in case you have to. Something like a force_direct() method (or a context manager?). WDYT?

A solution could be to extend NodeStrategy's API to ensure you're dealing with a direct node in case you have to.

What is the difference to a DirectNode then?

I think this should create direct node, and conditionally if a daemon is running query some information from that to not have to wait for "complete" discovery for the direct node.

What is the difference to a DirectNode then?

That it'd use the daemon outside the scope where you actually need it to be direct, e.g.:

with NodeStrategy(args) as node: # ... with node.force_direct(): node.subscribe()

I think this should create direct node, and conditionally if a daemon is running query some information from that to not have to wait for "complete" discovery for the direct node.

That sounds a lot like replicating NodeStrategy's implementation N times. I'd rather improve it in a single place.

See #499 for an alternative.

See #499 for an alternative.

This looks much better 👍

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2020-05-11T19:17:33Z

CI up to ros2topic, ros2service, ros2action, ros2node and ros2component:

Linux (likely a flake, can't reproduce locally)
Linux-aarch64
OSX
Windows

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

jacobperron

LGTM with green CI

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

dawonn-haval · 2020-05-12T01:07:15Z

Just wanted to chime in and mention that this patch resolves the cli instability for me across multiple hosts that I've been seeing for a long time.

I wonder if the same changes need to be made to the ros2bag package?

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2020-05-12T16:10:09Z

Running CI once again, got linter test failures due to recent flake8 updates.

clalancette · 2020-05-12T16:49:19Z

I might also suggest a full CI run on Linux, just to see what the system_tests stuff looks like with this in place.

dirk-thomas · 2020-05-12T16:51:48Z

I might also suggest a full CI run on Linux, just to see what the system_tests stuff looks like with this in place.

Which part of ros2cli do the system_tests use?

clalancette · 2020-05-12T17:49:20Z

Which part of ros2cli do the system_tests use?

I'm honestly not sure, but this is a big enough change that it makes me wary of the overnights. I'd prefer to see what it does on a full run, as I'd like not to regress at this point (we are actually getting pretty close to green for Linux CI, for instance).

dirk-thomas · 2020-05-12T17:56:44Z

I'd prefer to see what it does on a full run,

A more complete run always makes sense. But also only for downstream packages which actually depend on something in the ros2cli repo. And in system_tests the only package depending on ros2cli is test_security - so running CI on e.g. test_communication would be not helpful and a waste of time / resources.

hidmic · 2020-05-13T13:16:44Z

CI above ros2cli:

Linux

hidmic · 2020-05-13T19:44:57Z

@clalancette @dirk-thomas all test failures here are unrelated to this patch. I've sent separate PRs to fix flake8 errors and sros2 test failures. Do you want to move forward or do you want to wait for those to land and make another CI run, this time across platforms?

dirk-thomas

I am fine with it as is.

It would be good to investigate why the test failure happened in the PR build.

hidmic · 2020-05-13T20:14:53Z

It would be good to investigate why the test failure happened in the PR build.

It's a strange one, I cannot reproduce locally. It doesn't show on nightlies either.

aitazhixin · 2020-08-19T06:24:10Z

In dashing 0.7.11 version, 'Could not determine the type for the passed topic' also happens.

hidmic · 2020-08-19T13:47:22Z

@aitazhixin I don't think these changes were backported to Dashing. This patch (and connected ones) only extends and improves API, but I will say that anything bigger than a bugfix is less likely to be considered for a backport. @nuclearsandwich thoughts?

aitazhixin · 2020-08-20T03:35:36Z

@hidmic so, I need a version which is bigger than 0.9.3?

hidmic requested review from dirk-thomas, ivanpauno and jacobperron April 22, 2020 16:31

hidmic mentioned this pull request Apr 22, 2020

Make CLI more robust to discovery latency. #421

Closed

hidmic force-pushed the hidmic/robust-cli branch from fac0a49 to 02c8d4a Compare April 27, 2020 18:48

dirk-thomas reviewed Apr 27, 2020

View reviewed changes

hidmic self-assigned this May 7, 2020

hidmic mentioned this pull request May 11, 2020

Improve NodeStrategy to use the right node seamlessly. #499

Merged

hidmic added 5 commits May 11, 2020 16:15

Make ros2 topic verbs more robust to discovery latency.

e4ee5cd

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Make ros2 service verbs more robust to discovery latency.

7e56ab0

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Make ros2 node verbs more robust to discovery latency.

edb51fb

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Make ros2 component verbs more robust to discovery latency.

07d9773

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Make ros2 action verbs more robust to discovery latency.

d334eaa

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic force-pushed the hidmic/robust-cli branch from 02c8d4a to d334eaa Compare May 11, 2020 19:16

hidmic requested a review from dirk-thomas May 11, 2020 19:17

dirk-thomas reviewed May 11, 2020

View reviewed changes

hidmic mentioned this pull request May 11, 2020

[ros2component] Avoid long timeouts in ros2 component list #507

Closed

Address peer review comments.

6936367

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

jacobperron approved these changes May 11, 2020

View reviewed changes

Comment thread ros2topic/ros2topic/verb/list.py Outdated

Revert one last unrelated change.

ba6248b

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

dirk-thomas reviewed May 11, 2020

View reviewed changes

Comment thread ros2component/ros2component/api/__init__.py Outdated

dirk-thomas reviewed May 11, 2020

View reviewed changes

Comment thread ros2topic/ros2topic/verb/list.py

Address peer review comments.

5a28d9d

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

dirk-thomas approved these changes May 13, 2020

View reviewed changes

hidmic merged commit 2ea0bcc into master May 13, 2020

delete-merged-branch Bot deleted the hidmic/robust-cli branch May 13, 2020 20:16

hidmic mentioned this pull request May 14, 2020

ros2 topic echo should use the daemon to look up type names #488

Closed

jacobperron mentioned this pull request May 28, 2020

ros2 topic echo crashes with the error when trying to echo a nonexistent topic #519

Closed

jacobperron reviewed May 28, 2020

View reviewed changes

Comment thread ros2topic/ros2topic/verb/echo.py

Conversation

hidmic commented Apr 22, 2020

Uh oh!

hidmic commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hidmic commented Apr 27, 2020

Uh oh!

dirk-thomas commented Apr 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hidmic commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jacobperron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dawonn-haval commented May 12, 2020

Uh oh!

hidmic commented May 12, 2020

Uh oh!

clalancette commented May 12, 2020

Uh oh!

dirk-thomas commented May 12, 2020

Uh oh!

clalancette commented May 12, 2020

Uh oh!

dirk-thomas commented May 12, 2020

Uh oh!

hidmic commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hidmic commented May 13, 2020

Uh oh!

dirk-thomas left a comment

Choose a reason for hiding this comment

Uh oh!

hidmic commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

aitazhixin commented Aug 19, 2020

Uh oh!

hidmic commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

hidmic commented Apr 27, 2020 •

edited

Loading

hidmic Apr 28, 2020 •

edited

Loading

hidmic commented May 11, 2020 •

edited

Loading

hidmic commented May 13, 2020 •

edited

Loading

hidmic commented May 13, 2020 •

edited

Loading

hidmic commented Aug 19, 2020 •

edited

Loading