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

Restructure Pyconfig #1285

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Restructure Pyconfig #1285

wants to merge 1 commit into from

Conversation

A9isha
Copy link
Collaborator

@A9isha A9isha commented Feb 19, 2025

Description

Restructure pyconfig.py such that we can create multiple configs. This becomes important if we have a need from an RL workload to have training and inference configs which differ in sharding strategies among others.
We also now use OmegaConfig instead of yaml.safe_load and get its benefits

FIXES: b/396747968

Tests

Tested this code with Llama2-7b on v4-8

Checklist

Before submitting this PR, please make sure (put X in square brackets):

  • I have performed a self-review of my code.
  • I have necessary comments in my code, particularly in hard-to-understand areas.
  • I have run end-to-end tests tests and provided workload links above if applicable.
  • I have made or will make corresponding changes to the doc if needed.

@A9isha A9isha force-pushed the anisha-pyconfig branch 3 times, most recently from d943b05 to 99b807e Compare February 19, 2025 21:28
@A9isha A9isha changed the title Anisha pyconfig Restructure Pyconfig Feb 19, 2025
@A9isha A9isha marked this pull request as ready for review February 19, 2025 21:59
@A9isha A9isha requested review from richjames0 and shralex February 19, 2025 21:59
@shralex
Copy link
Collaborator

shralex commented Feb 20, 2025

if you'd like to add a unit test, you could for example test that invoking initialize twice creates two different configs, and that attempts to modify a config will result in an error

@A9isha
Copy link
Collaborator Author

A9isha commented Feb 20, 2025

if you'd like to add a unit test, you could for example test that invoking initialize twice creates two different configs, and that attempts to modify a config will result in an error

Done - thank you Alex!

Copy link
Collaborator

@khatwanimohit khatwanimohit left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@gobbleturk gobbleturk left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks Anisha!

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.

5 participants