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.

Sign up for GitHub

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

Jump to bottom

feat: new format for the controller metrics and operations #130

Merged
merged 1 commit into from
May 7, 2024
Merged

feat: new format for the controller metrics and operations #130

merged 1 commit into from
May 7, 2024

Conversation

HarikrishnanBalagopal
Copy link
Contributor

@HarikrishnanBalagopal HarikrishnanBalagopal commented Apr 23, 2024 •
edited
Loading

Description of the change

Standardized the format in the yaml files used by TrainerControllerCallback.

Related issue number

#129

How to verify the PR

We have tests which can be run with make test

Was the PR tested

Run make test

  • I have added >=1 unit test(s) for every new method I have added.
  • I have ensured all unit tests pass

kmehant
kmehant previously requested changes Apr 24, 2024
View reviewed changes
tuning/trainercontroller/callback.py Outdated Show resolved Hide resolved
alex-jw-brooks
alex-jw-brooks requested changes Apr 25, 2024
View reviewed changes
Copy link
Collaborator

@alex-jw-brooks alex-jw-brooks left a comment

Choose a reason for hiding this comment

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

HI @HarikrishnanBalagopal, thanks for the contribution. I think the schema change looks reasonable, but would like to decouple the schema change and the schema validation to keep things as atomic as possible, especially since the schema validation adds a new dependency. Would you be able to split this into two PRs?

@HarikrishnanBalagopal HarikrishnanBalagopal force-pushed the feat/changeformat branch 2 times, most recently from 7e91273 to 02f4b96 Compare April 25, 2024 21:22
@HarikrishnanBalagopal
Copy link
Contributor Author

HI @HarikrishnanBalagopal, thanks for the contribution. I think the schema change looks reasonable, but would like to decouple the schema change and the schema validation to keep things as atomic as possible, especially since the schema validation adds a new dependency. Would you be able to split this into two PRs?

@alex-jw-brooks Thanks for the review, I have removed the jsonschema dependency and schema validation code. PTAL

alex-jw-brooks
alex-jw-brooks requested changes May 2, 2024
View reviewed changes
Copy link
Collaborator

@alex-jw-brooks alex-jw-brooks left a comment

Choose a reason for hiding this comment

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

One small thing, but otherwise I think we can get this one merged, thanks!

tuning/trainercontroller/callback.py Outdated
@@ -74,7 +76,8 @@ def __init__(self, trainer_controller_config: Union[dict, str]):
if isinstance(trainer_controller_config, str):
if os.path.exists(trainer_controller_config):
with open(trainer_controller_config, "r", encoding="utf-8") as f:
self.trainer_controller_config = yaml.safe_load(f)
self.trainer_controller_config: dict = yaml.safe_load(f)
assert isinstance(self.trainer_controller_config, dict)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you change this to raise TypeError if not isinstance(self.trainer_controller_config, dict) instead of an assert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tuning/trainercontroller/callback.py
# Add metric instances to the events.
for event_name in obj.get_events():
for event_name in metric_handler.get_events():
Copy link
Collaborator

Choose a reason for hiding this comment

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

just a note in general - in the future it would be best to decouple minor changes and refactors from functional changes to keep PRs as small/atomic as possible, as it keeps the PRs easier to review and less likely to be in conflict with each other 🙂

Copy link
Contributor Author

@HarikrishnanBalagopal HarikrishnanBalagopal May 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I will keep that in mind for the other PRs.
I take it we can keep this change for now?

@HarikrishnanBalagopal HarikrishnanBalagopal force-pushed the feat/changeformat branch 2 times, most recently from 19ba188 to 151b407 Compare May 3, 2024 09:32
@HarikrishnanBalagopal
feat: new format for the controller metrics and operations
4cb3a89 
Signed-off-by: Harikrishnan Balagopal <harikrishmenon@gmail.com>
@alex-jw-brooks alex-jw-brooks dismissed kmehant’s stale review May 7, 2024 16:45

looks like this has been addressed - seems like I need to do this to merge

@alex-jw-brooks alex-jw-brooks merged commit 5c79fa0 into foundation-model-stack:main May 7, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex-jw-brooks alex-jw-brooks alex-jw-brooks approved these changes

@anhuong anhuong Awaiting requested review from anhuong anhuong is a code owner

@Ssukriti Ssukriti Awaiting requested review from Ssukriti Ssukriti is a code owner

@kmehant kmehant Awaiting requested review from kmehant kmehant is a code owner

@dushyantbehl dushyantbehl Awaiting requested review from dushyantbehl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HarikrishnanBalagopal @dushyantbehl @alex-jw-brooks @kmehant