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

Refactor: Move metrics logic to MetricLogger class #1315

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

Conversation

SurbhiJainUSC
Copy link
Collaborator

@SurbhiJainUSC SurbhiJainUSC commented Feb 26, 2025

Description

This PR initiates the refactoring of train.py. As part of refactoring effort, we will be:

  • Consolidating all the metrics-related code into a dedicated MetricLogger class (PR)
  • Moving some methods in train.py to a util module that can be re-used by other trainers (To be followed)

Tests

Ran MaxText/train.py with gcs_metrics=True and metrics_file="/tmp/maxtext_metrics.txt" configurations to ensure that the metrics were stored in a local file, GCS and TensorBoard.

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.

Copy link

Important

The terms of service for this installation has not been accepted. Please ask the Organization owners to visit the Gemini Code Assist Admin Console to sign it.

@SurbhiJainUSC SurbhiJainUSC requested a review from A9isha February 26, 2025 19:31
max_utils.close_summary_writer(writer)
record_goodput(recorder, config, recorder.record_job_end_time if recorder else None)
clear_buffered_metrics()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this clear no longer needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This method is basically setting the global variables to None. I have moved those global variables to class variables instead. AFAIK, python's garbage collector will automatically clear the memory occupied by that class object when program exits. So, this method shouldn't be required.
@gobbleturk - LMK if I am missing anything here.

Copy link
Collaborator

@gobbleturk gobbleturk Feb 27, 2025

Choose a reason for hiding this comment

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

I have seen issues when calling the main() of train.py mutiple times within the same python invocation, e.g. if you run a script like

  from train.py import main as train_main 
  train_main()
  train_main()

then the second call of train_main() will inherit the global variables (the metrics) of the end of the first unless they are explicitly cleared. I don't have much understanding of python garbage collection though. If we refactor train well enough perhaps this clear method is not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case, each invocation of train_main() will have its own metric logger object. All the metrics related data can only be accessed by the class object. So, IMO the second call of train_main() will not be able to access the metrics from the first train_main() call.

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.

LGTM, this is awesome thanks Surbhi!

I do think we still need the clear method as described in the comment though - but if you can run a test with two train_main calls in the same python script and see that the second run has fresh metrics perhaps it can be removed

Copy link
Collaborator

@bvandermoon bvandermoon left a comment

Choose a reason for hiding this comment

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

LGTM pending the outcome of the clear method discussion

@SurbhiJainUSC
Copy link
Collaborator Author

LGTM pending the outcome of the clear method discussion

Verified that two sequential train_main() function calls within the same script produced different sets of metrics.

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.

4 participants