Skip to content

Conversation

@sajadn
Copy link
Contributor

@sajadn sajadn commented Nov 20, 2025

  • add unit tests
  • very minor cleanings
  • add missing iter to the DiffusionDataModuleConfig

@copy-pr-bot
Copy link

copy-pr-bot bot commented Nov 20, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@sajadn
Copy link
Contributor Author

sajadn commented Nov 20, 2025

/ok to test fa1b884

@sajadn
Copy link
Contributor Author

sajadn commented Nov 28, 2025

/ok to test c376224

@sajadn sajadn requested a review from a team as a code owner November 28, 2025 16:12
@abhinavg4
Copy link
Contributor

/ok to test 3e93c2a

abhinavg4
abhinavg4 previously approved these changes Dec 1, 2025
Copy link
Contributor

@abhinavg4 abhinavg4 left a comment

Choose a reason for hiding this comment

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

Looks good. Let;s merge once tets pass

"output_params": ["-f", "mp4"],
}

print("video_save_path", video_save_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Good cleanup! Removing debug print statements keeps the output clean in production.

def build_datasets(self, context: DatasetBuildContext):
return self.dataset.train_dataloader(), self.dataset.val_dataloader(), self.dataset.test_dataloader()
return (
iter(self.dataset.train_dataloader()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice improvement! Wrapping dataloaders with iter() makes the interface more explicit and reduces potential confusion in downstream usage.

@sajadn
Copy link
Contributor Author

sajadn commented Dec 1, 2025

/ok to test f859fbb

Copy link
Contributor

@huvunvidia huvunvidia left a comment

Choose a reason for hiding this comment

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

Left a few comments.

DiTModelProvider: Configuration for the DiT-S model.
"""
return DiTModelProvider(
return DiTXLModelProvider(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: In docstring it notes "DiT-S" but we provide DiTXLModelProvider.

"mediapy>=1.2.4",
"megatron-bridge",
"wandb[media]>=0.23.0",
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure, are mediapy and wandb included in OSRB?
Any dependency needs to be approved for open-source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, this is a great question. I'm not sure about that.

@sajadn
Copy link
Contributor Author

sajadn commented Dec 3, 2025

/ok to test 99ada83

Copy link
Contributor

@abhinavg4 abhinavg4 left a comment

Choose a reason for hiding this comment

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

LOkks good

@sajadn sajadn enabled auto-merge (squash) December 3, 2025 02:02
@pablo-garay
Copy link
Collaborator

Approved from Automation perspective

@abhinavg4
Copy link
Contributor

/ok to test f007a7f

@sajadn sajadn merged commit 1061749 into main Dec 3, 2025
15 checks passed
lbliii pushed a commit that referenced this pull request Dec 3, 2025
* edm and data preprocess tests.

Signed-off-by: sajadn <[email protected]>

* Minor cleanings for DiT.

Signed-off-by: Sajad Norouzi <[email protected]>

* add dit unit test.

Signed-off-by: Sajad Norouzi <[email protected]>

* add iter to the DiffusionDataModule.

Signed-off-by: sajadn <[email protected]>

* add missing copyright.

Signed-off-by: sajadn <[email protected]>

* use 'no caption' if caption is not present.

Signed-off-by: sajadn <[email protected]>

* fix dit inference bug. Add wanbd to inference code.

Signed-off-by: sajadn <[email protected]>

* update the DiT configs to be aligned with the original paper.

Signed-off-by: sajadn <[email protected]>

* add wandb[video] and mediapy to uv.

Signed-off-by: sajadn <[email protected]>

* adjust pos_ids in mock_dataset to have batch dimension, fuse adaLN layers, use DiTSelfAttention.

Signed-off-by: sajadn <[email protected]>

* fix the diffusion sample size bug.

Signed-off-by: sajadn <[email protected]>

* fix broken tests.

Signed-off-by: sajadn <[email protected]>

---------

Signed-off-by: sajadn <[email protected]>
Signed-off-by: Sajad Norouzi <[email protected]>
Co-authored-by: Abhinav Garg <[email protected]>
Signed-off-by: Lawrence Lane <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants