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

Fix export of configuration parameters to Weights and Biases #10995

Merged
merged 7 commits into from
Nov 5, 2024

Conversation

soluwalana
Copy link
Collaborator

@soluwalana soluwalana commented Oct 22, 2024

What does this PR do ?

This PR corrects an error in ModelPT.py that only partially converts the _hparams_initial from an OmegaConf object to a dictionary before exporting to the various loggers.

This was particularly an issue with search functionality based on metadata in Weights and Biases due to the cfg object being a string representation of the OmegaConf object.

Collection: Core

Changelog

  • Adds an additional check and conversion of the OmegaConf self._cfg to ensure that Logger.log_hyperparameters from Pytorch Lightning is guaranteed to find a JSON compatible structure at L1650 of core/classes/ModelPT.py

Reference Images

Prior to fix

cfg is recorded as a str(OmegaConf.DictConfig) in Weights and Biases

Screenshot 2024-10-22 at 11 21 02

After fix

cfg is correctly searchable in Weights and Biases

Screenshot 2024-10-22 at 11 21 17

GitHub Actions CI

The Jenkins CI system has been replaced by GitHub Actions self-hosted runners.

The GitHub Actions CI will run automatically when the "Run CICD" label is added to the PR.
To re-run CI remove and add the label again.
To run CI on an untrusted fork, a NeMo user with write access must first click "Approve and run".

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you add or update any necessary documentation?
  • Does the PR affect components that are optional to install? (Ex: Numba, Pynini, Apex etc)
    • Reviewer: Does the PR have correct import guards for all optional libraries?

PR Type:

  • New Feature
  • Bugfix
  • Documentation

If you haven't finished some of the above items you can still open "Draft" PR.

Who can review?

Anyone in the community is free to review the PR once the checks have passed.
Contributor guidelines contains specific people who can review PRs to various areas.

Additional Information

  • Related to # (issue) LLM-5007

@github-actions github-actions bot added the core Changes to NeMo Core label Oct 22, 2024
Add test to ensure json serializability of hparams_init after restore

Signed-off-by: Sam Oluwalana <[email protected]>
titu1994
titu1994 previously approved these changes Oct 29, 2024
Copy link
Collaborator

@titu1994 titu1994 left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks for the fix !

ericharper
ericharper previously approved these changes Oct 29, 2024
Copy link
Collaborator

@ericharper ericharper left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@ericharper ericharper enabled auto-merge (squash) October 29, 2024 23:01
@soluwalana
Copy link
Collaborator Author

@ericharper @titu1994 can you trigger the Nemo_CICD_Test, cicd-test-container-setup, and gpu-test actions?

auto-merge was automatically disabled November 5, 2024 04:50

Head branch was pushed to by a user without write access

@soluwalana soluwalana dismissed stale reviews from ericharper and titu1994 via 2d556c0 November 5, 2024 04:50
Copy link
Collaborator

@ericharper ericharper left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

Copy link
Collaborator

@titu1994 titu1994 left a comment

Choose a reason for hiding this comment

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

Thanks !

@ericharper ericharper merged commit a08754d into NVIDIA:main Nov 5, 2024
158 of 159 checks passed
lilyw97 pushed a commit to lilyw97/NeMo that referenced this pull request Nov 13, 2024
…10995)

* Fix export of configuration parameters to weights and biases

Add test to ensure json serializability of hparams_init after restore

Signed-off-by: Sam Oluwalana <[email protected]>

* Function was moved in recent versions

Signed-off-by: Sam Oluwalana <[email protected]>

* Apply isort and black reformatting

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

---------

Signed-off-by: Sam Oluwalana <[email protected]>
Signed-off-by: soluwalana <[email protected]>
Co-authored-by: soluwalana <[email protected]>
HuiyingLi pushed a commit to HuiyingLi/NeMo that referenced this pull request Nov 15, 2024
…10995)

* Fix export of configuration parameters to weights and biases

Add test to ensure json serializability of hparams_init after restore

Signed-off-by: Sam Oluwalana <[email protected]>

* Function was moved in recent versions

Signed-off-by: Sam Oluwalana <[email protected]>

* Apply isort and black reformatting

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

---------

Signed-off-by: Sam Oluwalana <[email protected]>
Signed-off-by: soluwalana <[email protected]>
Co-authored-by: soluwalana <[email protected]>
yashaswikarnati pushed a commit that referenced this pull request Nov 21, 2024
* Fix export of configuration parameters to weights and biases

Add test to ensure json serializability of hparams_init after restore

Signed-off-by: Sam Oluwalana <[email protected]>

* Function was moved in recent versions

Signed-off-by: Sam Oluwalana <[email protected]>

* Apply isort and black reformatting

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

---------

Signed-off-by: Sam Oluwalana <[email protected]>
Signed-off-by: soluwalana <[email protected]>
Co-authored-by: soluwalana <[email protected]>
XuesongYang pushed a commit to paarthneekhara/NeMo that referenced this pull request Jan 18, 2025
…10995)

* Fix export of configuration parameters to weights and biases

Add test to ensure json serializability of hparams_init after restore

Signed-off-by: Sam Oluwalana <[email protected]>

* Function was moved in recent versions

Signed-off-by: Sam Oluwalana <[email protected]>

* Apply isort and black reformatting

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

---------

Signed-off-by: Sam Oluwalana <[email protected]>
Signed-off-by: soluwalana <[email protected]>
Co-authored-by: soluwalana <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Changes to NeMo Core Run CICD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants