primitives: deprecate SignalLogger and friends #15594

rpoyner-tri · 2021-08-10T20:48:48Z

This is step 4 of 4 to deprecate and replace SignalLogger and SignalLog
with a logging system that avoids threading hazards (SignalLogger) and
slow memory management (SignalLog).

The patch deprecates SignalLog, SignalLogger, and LogOutput, and
replaces them within Drake by VectorLog, VectorLogSink and
LogVectorOutput, respectively.

This change is

rpoyner-tri

+@SeanCurtis-TRI has graciously agreed to feature review.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)

a discussion (no related file):
Unresolved anzu angst: this PR deprecates the old solution, and so motivates a desire to port to the new in downstream projects. I've done this exercise several times during work on the PR train, and reliably get test failures in one of the anzu test programs. Does that block this PR at all, or can we just roll and debug the migration problems separately?

A stale anzu migration example is at anzu::#7361. I will update it and see how it fares now.

jwnimmer-tri · 2021-08-10T21:12:17Z

a discussion (no related file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

Unresolved anzu angst: this PR deprecates the old solution, and so motivates a desire to port to the new in downstream projects. I've done this exercise several times during work on the PR train, and reliably get test failures in one of the anzu test programs. Does that block this PR at all, or can we just roll and debug the migration problems separately?

A stale anzu migration example is at anzu::#7361. I will update it and see how it fares now.

It's valid to work on this PR and an Anzu PR in parallel, and it's valid to land a Drake PR without perfecting a related Anzu PR as a precondition of the merge to Drake master.

However, we do need maintain the invariant that bazel test //... against Anzu master + Drake master is almost always passing, because using an unpinned Drake + Anzu master is an important, supported use case. We do allow for the invariant to be broken for a few hours to allow for staggered merges, but if the Anzu fallout isn't fixed within those few hours, the response will be to revert the Drake PR, in order to restore the invariant in a timely manner.

rpoyner-tri

Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri and @SeanCurtis-TRI)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It's valid to work on this PR and an Anzu PR in parallel, and it's valid to land a Drake PR without perfecting a related Anzu PR as a precondition of the merge to Drake master.

However, we do need maintain the invariant that bazel test //... against Anzu master + Drake master is almost always passing, because using an unpinned Drake + Anzu master is an important, supported use case. We do allow for the invariant to be broken for a few hours to allow for staggered merges, but if the Anzu fallout isn't fixed within those few hours, the response will be to revert the Drake PR, in order to restore the invariant in a timely manner.

I don't think it will come to that as of this PR -- this just deprecates.

SeanCurtis-TRI

Generally fine. The big issue is the breaking change to pyplot_visualizer (see below).

Reviewed 31 of 31 files at r1, all commit messages.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri and @rpoyner-tri)

a discussion (no related file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

I don't think it will come to that as of this PR -- this just deprecates.

@jwnimmer-tri I've got a related question. Can I do a PR with two commits: one updates Drake the second applies the patch?

bindings/pydrake/systems/pyplot_visualizer.py, line 116 at r1 (raw file):

        return ani

    def animate(self, log, resample=True, **kwargs):

nit: This is a breaking change and defies the deprecation. The user doesn't have the option to ignore deprecation warnings and resolve it some time in the future, their code will immediately be broken.

During the deprecation period, you should test to see if log is VectorLog or SignalLogger and act accordingly. Because you only deprecated the constructor, you can safely exercise the API without spewing additional warnings. And then the SignalLogger branch gets with the class.

bindings/pydrake/systems/test/primitives_test.py, line 116 at r1 (raw file):

    @numpy_compare.check_nonsymbolic_types
    def test_signal_logger(self, T):
        with catch_drake_warnings(expected_count=3):

BTW This is a pretty big hammer. If anything else needs to get deprecated relative to this test in the next three months, it'll be a nuisance to figure out which of these calls actually triggered the warnings.

bindings/pydrake/systems/test/pyplot_visualizer_test.py, line 76 at r1 (raw file):

        builder = DiagramBuilder()
        system = builder.AddSystem(SimpleContinuousTimeSystem())
        logger = builder.AddSystem(VectorLogSink(1))

nit Based on my comment about not breaking the animate() method, this test will need to preserve the SignalLogger-flavored version of this test.

systems/discrete_systems.h, line 86 at r1 (raw file):

  auto example = builder.AddSystem<ExampleDiscreteSystem>();
  auto logger = LogVectorOutput(example->GetOutputPort("Sn"), &builder);
  logger->SetPublishPeriod(ExampleDiscreteSystem::kPeriod);

nit: Is this method still a thing? I suspect not. I think you need to respell the example of calling LogVectorOutput to include the periodic event.

systems/primitives/signal_log.h, line 19 at r1 (raw file):

 */
template <typename T>
class DRAKE_DEPRECATED("2021-11-01", "Use VectorLog instead.") SignalLog {

nit: This should be December.

systems/primitives/signal_logger.h, line 56 at r1 (raw file):

/// @ingroup primitive_systems
template <typename T>
class DRAKE_DEPRECATED("2021-11-01", "Use VectorLogSink instead.")

nit: December (multiple times).

systems/primitives/test/discrete_derivative_test.cc, line 53 at r1 (raw file):

  builder.Connect(ppsource->get_output_port(), deriv->get_input_port());
  // Evaluate the outputs at a distinct sampling interval.
  auto sink = LogVectorOutput(deriv->get_output_port(), &builder,

BTW I've seen a lot of "logger" in this PR, but I think this is the first "sink" I've seen. Intentional?

jwnimmer-tri

Reviewable status: 7 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers (waiting on @rpoyner-tri and @SeanCurtis-TRI)

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

@jwnimmer-tri I've got a related question. Can I do a PR with two commits: one updates Drake the second applies the patch?

@rpoyner-tri Aha, my answer was incomplete. Another invariant is that we never spam the console (e.g., with deprecation warnings) during a build, or when running a python program on the command line. However, that doesn't imply that you need to do the Anzu porting and hunt down the bug immediately. Instead, the Anzu PR you'd need to submit ASAP is one that muted the deprecation warnings.

@SeanCurtis-TRI Yes, that is a supported workflow, and in fact is the only way possible when Drake has certain kinds of breaking changes. When a Drake upgrade does not have breaking changes, though, I try to encourage people to first do the sha bump, merge that, and then submit the follow-up separately. That proves that there we no breaking changes.

SeanCurtis-TRI

Reviewable status: 7 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri and @rpoyner-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

@rpoyner-tri Aha, my answer was incomplete. Another invariant is that we never spam the console (e.g., with deprecation warnings) during a build, or when running a python program on the command line. However, that doesn't imply that you need to do the Anzu porting and hunt down the bug immediately. Instead, the Anzu PR you'd need to submit ASAP is one that muted the deprecation warnings.

@SeanCurtis-TRI Yes, that is a supported workflow, and in fact is the only way possible when Drake has certain kinds of breaking changes. When a Drake upgrade does not have breaking changes, though, I try to encourage people to first do the sha bump, merge that, and then submit the follow-up separately. That proves that there we no breaking changes.

Thanks (I have some minor clean up related to #15534).

rpoyner-tri

Reviewable status: 6 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers (waiting on @rpoyner-tri)

systems/discrete_systems.h, line 86 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Is this method still a thing? I suspect not. I think you need to respell the example of calling LogVectorOutput to include the periodic event.

yup, looks stale and busted.

rpoyner-tri

+(status: do not merge) holding until I work out an accompanying Anzu PR. Other review can continue.

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @SeanCurtis-TRI)

SeanCurtis-TRI

+@sammy-tri for platform review, as per schedule.

Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform), labeled "do not merge" (waiting on @rpoyner-tri and @sammy-tri)

bindings/pydrake/systems/pyplot_visualizer.py, line 135 at r2 (raw file):

        if log_is_signal_logger and not self._warned_signal_logger_deprecated:
            _warn_deprecated("SignalLogger is deprecated. Use VectorLog and"
                             " VectorLogSink instead.", date="2021-12-01")

nit: In this particular case, I believe reference to VectorLogSink is misleading -- that would not be a valid parameter type.

rpoyner-tri

Reviewable status: LGTM missing from assignee sammy-tri(platform), labeled "do not merge" (waiting on @sammy-tri and @SeanCurtis-TRI)

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Thanks (I have some minor clean up related to #15534).

anzu::#7361 is behaving better -- not quite perfect yet.

SeanCurtis-TRI

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: LGTM missing from assignee sammy-tri(platform), labeled "do not merge" (waiting on @sammy-tri)

bindings/pydrake/systems/pyplot_visualizer.py, line 135 at r2 (raw file):
This seemed to go in exactly the opposite direction of what I intended. In the documentation above you have:

log: A reference to a pydrake.systems.primitives.VectorLog...

In this version of thewarning, you are pointing them toward VectorLogSink which is not what is documented and not what should be passed (which this is now explicitly lying about).

My original comment was to hint that you should just excise reference to VectorLogSink (i.e., "SignalLogger is deprecated, Use VectorLog instead") and rely on documentation and context for the user to figure out where a VectorLog comes from.

Discussion of VectorLogSink in this deprecation warning, I feel, requires far more elaboration than is worthwhile.

SeanCurtis-TRI

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: LGTM missing from assignee sammy-tri(platform), labeled "do not merge" (waiting on @sammy-tri)

rpoyner-tri

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform), labeled "do not merge" (waiting on @rpoyner-tri and @sammy-tri)

examples/acrobot/spong_sim.cc, line 123 at r4 (raw file):

  Output output;
  output.x_tape = state_logger->FindLog(simulator.get_context()).data();

one last bit of fiddling here to minimize divergence with the twin file in anzu.

rpoyner-tri

Reviewable status: LGTM missing from assignee sammy-tri(platform), labeled "do not merge" (waiting on @sammy-tri and @SeanCurtis-TRI)

a discussion (no related file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

anzu::#7361 is behaving better -- not quite perfect yet.

The anzu patch is ready to go; just awaiting drake bump and various CI cycles. I'm content to hold this patch until the anzu::#7361 is merged.

SeanCurtis-TRI

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: LGTM missing from assignee sammy-tri(platform), labeled "do not merge" (waiting on @sammy-tri)

rpoyner-tri

-(status: do not merge) anzu patch is merged.

Reviewable status: LGTM missing from assignee sammy-tri(platform), labeled "do not merge" (waiting on @sammy-tri)

sammy-tri

Reviewed 22 of 31 files at r1, 7 of 8 files at r2, 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform) (waiting on @rpoyner-tri)

systems/discrete_systems.h, line 95 at r5 (raw file):

  // Print out the contents of the log.
  const auto& log = logger.FindLog(simulator.get_context());
  for (int n = 0; n < log->sample_times().size(); ++n) {

Should the log-> examples in this comment now read log.?

rpoyner-tri

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform) (waiting on @rpoyner-tri)

systems/discrete_systems.h, line 95 at r5 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Should the log-> examples in this comment now read log.?

good catch.

Closes: RobotLocomotion#10228 This is step 4 of 4 to deprecate and replace SignalLogger and SignalLog with a logging system that avoids threading hazards (SignalLogger) and slow memory management (SignalLog). The patch deprecates SignalLog, SignalLogger, and LogOutput, and replaces them within Drake by VectorLog, VectorLogSink and LogVectorOutput, respectively.

rpoyner-tri

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform) (waiting on @sammy-tri and @SeanCurtis-TRI)

systems/discrete_systems.h, line 95 at r5 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

good catch.

Done.

sammy-tri

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),SeanCurtis-TRI(platform) (waiting on @rpoyner-tri)

jwnimmer-tri · 2021-11-02T22:12:30Z

examples/manipulation_station/end_effector_teleop_sliders.py

        builder.Connect(station.GetOutputPort("iiwa_velocity_estimated"),
                        iiwa_velocities.get_input_port(0))
    else:
        iiwa_velocities = None

    diagram = builder.Build()
    simulator = Simulator(diagram)
+    iiwa_velocities_log = iiwa_velocities.FindLog(simulator.get_context())


This line crashes (cannot call FindLog on None) when run in non-test mode.

rpoyner-tri assigned SeanCurtis-TRI Aug 10, 2021

rpoyner-tri commented Aug 10, 2021

View reviewed changes

SeanCurtis-TRI reviewed Aug 10, 2021

View reviewed changes

jwnimmer-tri reviewed Aug 10, 2021

View reviewed changes

SeanCurtis-TRI reviewed Aug 10, 2021

View reviewed changes

rpoyner-tri commented Aug 10, 2021

View reviewed changes

rpoyner-tri force-pushed the vls-replace branch from 4c723ea to 4255ccc Compare August 10, 2021 23:57

rpoyner-tri added the status: do not merge label Aug 11, 2021