Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introducing BatchBlock #1192

Merged
merged 12 commits into from
Jul 11, 2023
Merged

Introducing BatchBlock #1192

merged 12 commits into from
Jul 11, 2023

Conversation

marcromeyn
Copy link
Contributor

@marcromeyn marcromeyn commented Jul 10, 2023

Goals ⚽

BatchBlock will be used inside the Model to create the Batch object. It's also useful for things like masking & padding.

@marcromeyn marcromeyn added enhancement New feature or request area/pytorch labels Jul 10, 2023
@marcromeyn marcromeyn requested a review from sararb July 10, 2023 08:24
@marcromeyn marcromeyn self-assigned this Jul 10, 2023
@github-actions
Copy link

Documentation preview

https://nvidia-merlin.github.io/models/review/pr-1192

@marcromeyn marcromeyn marked this pull request as ready for review July 10, 2023 08:46
Copy link
Contributor

@sararb sararb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good to me! I've just left some minor comments/questions

A dictionary containing all the flattened features, targets, and sequences.
"""
flat_dict: Dict[str, torch.Tensor] = self._flatten()
dummy_tensor = torch.tensor(0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why we need the dummy_tensor variable?

Copy link
Contributor Author

@marcromeyn marcromeyn Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never use it, but we need to have it in order to keep the type of the Dict[str, torch.Tensor]. We could store the original value but that might take more memory, so that's why I added the dummy. We only care about the keys of the original inputs.

merlin/models/torch/models/base.py Show resolved Hide resolved
merlin/models/torch/predict.py Outdated Show resolved Hide resolved
merlin/models/torch/transforms/sequences.py Outdated Show resolved Hide resolved
result = batch.flatten_as_dict(input_batch)
assert len(result) == 9 # input keys are considered
assert (
len([k for k in result if k.startswith("inputs.")]) == 4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the difference between inputs. and features. keys ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inputs is what went into the batch-transformation, used for the mechanism to restore values of the batch that were not transformed by any of the branches.


def test_in_parallel(self):
feat, target = torch.tensor([1, 2]), torch.tensor([3, 4])
outputs = module_utils.module_test(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to how we can now create different preprocessing blocks for each subset of the inputs! I love it!!

@marcromeyn marcromeyn merged commit 3431321 into main Jul 11, 2023
@marcromeyn marcromeyn deleted the torch/batch-block branch July 11, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants