Skip to content

Conversation

@mvinci12
Copy link
Contributor

@mvinci12 mvinci12 commented Nov 7, 2025

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Contributor

@KeitaW KeitaW left a comment

Choose a reason for hiding this comment

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

Nice work! but let's not add submodule.

@KeitaW
Copy link
Contributor

KeitaW commented Nov 7, 2025

The directory structure under test case is formatted as framework/library/testcase (ex: https://github.com/aws-samples/awsome-distributed-training/tree/main/3.test_cases/pytorch/trl/grpo, https://github.com/aws-samples/awsome-distributed-training/tree/main/3.test_cases/pytorch/picotron/SmolLM-1.7B. Let's employ the same structure ( pytorch/verl/rlvr ?).

@mvinci12
Copy link
Contributor Author

mvinci12 commented Nov 9, 2025

lmk - thanks keita!

@KeitaW
Copy link
Contributor

KeitaW commented Nov 11, 2025

Copy link
Contributor

@KeitaW KeitaW left a comment

Choose a reason for hiding this comment

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

Left some additional minor comments.

Copy link
Contributor

@KeitaW KeitaW left a comment

Choose a reason for hiding this comment

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

I've stopped blocking this PR from merging, but please consider addressing the remaining comments before merging.

@KeitaW
Copy link
Contributor

KeitaW commented Nov 19, 2025

Thanks again for the contribution!

@mvinci12 mvinci12 merged commit 2760fa7 into main Nov 19, 2025
4 checks passed
@mvinci12 mvinci12 deleted the rlvr branch November 19, 2025 23:10
@nghtm
Copy link
Contributor

nghtm commented Nov 21, 2025

Nice!

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.

3 participants