perf(action): jitter the DROID composite instead of the three full-size views - #219
Open
harshitwandhare wants to merge 3 commits into
Open
perf(action): jitter the DROID composite instead of the three full-size views#219harshitwandhare wants to merge 3 commits into
harshitwandhare wants to merge 3 commits into
Conversation
…ze views DROID concat_view SFT ran the whole augmentation pipeline, crop, resize and ColorJitter, on wrist, left and right concatenated at full size, then downscaled the two exterior views by 2x while tiling them into the composite. ColorJitter therefore paid for 3H x W pixels per frame to produce a composite that only keeps 3H/2 x W of them. Splitting the pipeline around the composition moves the jitter onto the composite. The random crop and resize have to stay per view, since cropping the composite would cut across the tile boundaries, so only the colour op moves. Both orderings draw one set of colour factors and apply it to every view, so the views still agree on lighting and colour diversity per sample is unchanged; the exterior views are now jittered after downscaling rather than before, which for a random augmentation is a re-parameterization rather than a different distribution. _compose_multi_view, 16 frames at 180x320 per view, float32, 3 runs of 20 calls: before 193.3 / 188.7 / 189.4 ms after 96.8 / 102.0 / 102.1 ms about 1.9x on the method, close to the 2x the pixel count predicts. The uint8-first half of the suggestion in NVIDIA#174 is deliberately not included: measured separately it made the reordered path 1.35x slower, because ColorJitter in torchvision 0.20.1 round-trips uint8 through float for the hue and saturation ops. Adds droid_lerobot_dataset_test.py covering the tiling, that augmentation preserves the layout, that one colour draw covers all three views, and that the augmentor is not a no-op. Signed-off-by: Harshit Wandhare <harshitwandhare45@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #174, the reorder you proposed as the cheaper first step.
What was slow
_compose_multi_viewran crop, resize and ColorJitter oncat([wrist, left, right])at full size, then downscaled the two exterior views by 2x while tiling them. So the jitter paid for3H x Wpixels per frame to build a composite that only keeps3H/2 x Wof them. Half that work was thrown away by the next line.What the change does
Splits the pipeline around the composition. Crop and resize stay per view and run first, because cropping the composite would cut across the tile boundaries. Only ColorJitter moves, onto the composite.
Both orderings draw one set of colour factors and apply it to every view, so the views still agree on lighting and per-sample colour diversity is unchanged. What changes is that the exterior views are jittered after downscaling rather than before. For a random augmentation that is a re-parameterization rather than a different distribution, which is the same reasoning you gave for the device swap in point (1).
Numbers
Measured after syncing onto
mainat #223. The branch has since mergedmainagain at #226; that sync touched inference and MoT files only, none of them in this path, and the directory run below is from the current head._compose_multi_viewcalled directly ona bare instance, 16 frames at 180x320 per view, float32,
torch.set_num_threads(4),20 calls per round after a warmup. The two variants alternate round by round, so machine
load drifts across both sides rather than favouring whichever ran first:
About 1.7x on the whole method. The sets do not overlap. This measures the full method
rather than an isolated ColorJitter call, so it carries the crop, resize, interpolate and
cat that did not change. That is why it lands short of the 2x the pixel count predicts.
An earlier run on this branch read 1.9x from three runs per side taken one side after the
other. Alternating the rounds gives 1.7x and I trust that number more, so it replaces the
first one here.
Output shape is identical either way,
(16, 3, 270, 320).The uint8 half is deliberately not here
You suggested casting to uint8 ahead of the jitter for another 4x off the memory traffic. Measured separately, that made the reordered path 1.35x slower, not faster:
ColorJitterin torchvision 0.20.1 round-trips uint8 through float internally for the hue and saturation ops, so the conversion cost eats the bandwidth win. Numbers are in the issue thread. Happy to revisit if it behaves differently on GPU, but the workers being optimised here are CPU-bound.Tests
New
droid_lerobot_dataset_test.py, fourL0tests: the tiling itself, that augmentation preserves shape and does not bleed across tiles, that one colour draw covers all three views, and that the augmentor is not silently a no-op.The three behavioural ones pin what the reorder must not change, so I ran them against
mainfirst, with the helper adapted to the pre-change attribute name, and they pass there too:and on this branch:
Whole directory, to catch anything that read the old attribute:
ruff checkandruff format --checkclean on both files.What I could not run
pre-commit run --files <the two files>passes every hook that applies exceptuv-lock-script, which fails on this machine because the hook shells out to/bin/bash. That looks like a Windows environment problem on my side rather than anything about this change, but I have not proved that, so flagging it rather than claiming the gate is green.gpu-tests.ymlneeds a GPU runner and thecu128-trainextras, so it has not run here.Correcting something an earlier version of this description said. I claimed the new test file was not in the list that workflow runs and offered to wire it in. That was wrong. The
unittestjob runs the whole colocated suite in one invocation and its own comment says new tests are picked up automatically:The files named individually elsewhere in that workflow are the torchrun and deselect special cases, not the general path. Collection confirmed locally under the same command:
So nothing needs wiring. The four are CPU-only and run on that job alongside the rest of the colocated suite.