batched transform

[ijsnow]

Description of the change

Add a transform that applies each transform with a single traverse of each object rather than traversing an object for each transform. The hope is that this will result in less CPU usage since the number of nodes in an object will almost always be larger than the number of transforms and in the case that its not true, performance difference will be negligible.

This is an opt in feature with the batch_transforms field on the settings object, but all tests pass when using the BatchedTransform manually.

Type of change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Maintenance
• New release

Related issues

Shortcut stories and GitHub issues (delete irrelevant)

• Fix [SC-123195]
• Fix notifier isn't sending a "platform" param... should it be "python"? #1

Checklists

Development

• Lint rules pass locally
• The code changed/added as part of this pull request has been covered with tests
• All tests related to the changed code pass in development

Code review

• This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
• "Ready for review" label attached to the PR and reviewers assigned
• Issue from task tracker has a link to this pull request
• Changes have been reviewed by at least one other engineer

[danielmorell]

This is related to #412.

[ijsnow]

Do we care about the depth of a transformed node changing? Only asking because the existing tests pass using the batched transform. I wrote a test case that isn't supported by this change.

def test_switching_depth(self):
    class BoxedNumberTransformer(Transform):
        def transform_number(self, o, key=None):
            return {"num": o}

    class NumberToStringTransformer(Transform):
        def transform_number(self, o, key=None):
            return str(o)

    input = ("hello", {"there": 0})

    got = transform(
        input,
        [BoxedNumberTransformer(), NumberToStringTransformer()],
        batch_transforms=True,
    )

    self.assertEqual(got, ("hello", {"there": {"num": "0"}}))

I'm trying to get a nested traverse call to work but its seeming a little complicated.

[shortcut-integration]

This pull request has been linked to Shortcut Story #123195: pyrollbar: resource usage problems.

[mudetroit]

Do we care about the depth of a transformed node changing? Only asking because the existing tests pass using the batched transform. I wrote a test case that isn't supported by this change.

I feel like this is probably okay, but interested on @danielmorell's take on it. I feel like the behavior shown is what I would actually expect but perhaps I am wrong.

[danielmorell]

Do we care about the depth of a transformed node changing? Only asking because the existing tests pass using the batched transform. I wrote a test case that isn't supported by this change.

@ijsnow If I understand correctly, the test you show is failing with the changes in this PR?

[danielmorell]

I was able to do some profiling and performance testing. Initially this does not look a lot different from the numbers I was seeing before. but I need to make sure that I'm not messing it up somehow.

[ijsnow]

@danielmorell

@ijsnow If I understand correctly, the test you show is failing with the changes in this PR?

Yes and it looks like a test that should pass, however if I ensure that the change is used by all usages in the tests, all the existing tests pass, which is the only reason I'm asking if it is intended to be supported.

Do we have any way of knowing how many custom transforms are in use in the wild? I'm giving it another quick pass to try to get that test to work but since its behind a flag in the settings maybe we could just document that caveat, if it indeed does help with performance.

[danielmorell]

👍