Load ub_cfg from hydra config #7003

jbaczek · 2023-07-10T16:14:16Z

What does this PR do ?

Remove the need to load the yaml module inside GPT model and use hydra config instead.

Collection: nlp

Changelog

For ub_tp_comm_overlap_cfg config field you no longer provide a path to the config but the whole config instead.

Usage

If you add to your main config:

defaults:
  [...]
  - optional [email protected]_tp_comm_overlap_cfg:

then you can load the config from a known location like this:
++ [email protected]_tp_comm_overlap_cfg=<tp_config}

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

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)

Signed-off-by: Jan Baczek <[email protected]>

jbaczek · 2023-07-17T15:22:26Z

@ericharper can you review this PR?

for more information, see https://pre-commit.ci

Signed-off-by: Jan Baczek <[email protected]>

for more information, see https://pre-commit.ci

erhoo82 · 2023-07-24T16:01:33Z

LGTM Please assign me and @ericharper as the reviewers.

erhoo82

LGTM

nemo/collections/nlp/models/language_modeling/megatron_gpt_model.py

erhoo82 · 2023-07-24T16:21:05Z

@ericharper Can you review this PR? This looks good to me but should be good to have your feedback as well.

erhoo82 · 2023-07-31T04:40:59Z

@jbaczek
I tested and I see it didn't work. I checked that correct yaml file is passed without yaml. I printed but not a dict was passed but just text.

jbaczek · 2023-07-31T07:43:19Z

@erhoo82 can you elaborate? What command did you type? What is your conf directory structure?

Signed-off-by: Jan Baczek <[email protected]>

Signed-off-by: yaoyu-33 <[email protected]>

for more information, see https://pre-commit.ci

Signed-off-by: Jan Baczek <[email protected]>

titu1994

Thanks for the guard !

ericharper

LGTM. Thanks!

* Pass tp config via hydra Signed-off-by: Jan Baczek <[email protected]> * Remove self.ub_cfgs field - it isn't used anywhere else Signed-off-by: Jan Baczek <[email protected]> * Allow tp_overlap tree substitution in hydra config Signed-off-by: Jan Baczek <[email protected]> * Add warning in case of usage of the default tp config * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Change warning message Signed-off-by: Jan Baczek <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Add compute capability resolver Signed-off-by: Jan Baczek <[email protected]> * Bugfix Signed-off-by: Jan Baczek <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add guards to pynvml import Signed-off-by: Jan Baczek <[email protected]> --------- Signed-off-by: Jan Baczek <[email protected]> Signed-off-by: yaoyu-33 <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: yaoyu-33 <[email protected]>

* Pass tp config via hydra Signed-off-by: Jan Baczek <[email protected]> * Remove self.ub_cfgs field - it isn't used anywhere else Signed-off-by: Jan Baczek <[email protected]> * Allow tp_overlap tree substitution in hydra config Signed-off-by: Jan Baczek <[email protected]> * Add warning in case of usage of the default tp config * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Change warning message Signed-off-by: Jan Baczek <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Add compute capability resolver Signed-off-by: Jan Baczek <[email protected]> * Bugfix Signed-off-by: Jan Baczek <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add guards to pynvml import Signed-off-by: Jan Baczek <[email protected]> --------- Signed-off-by: Jan Baczek <[email protected]> Signed-off-by: yaoyu-33 <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: yaoyu-33 <[email protected]> Signed-off-by: dorotat <[email protected]>

* Pass tp config via hydra Signed-off-by: Jan Baczek <[email protected]> * Remove self.ub_cfgs field - it isn't used anywhere else Signed-off-by: Jan Baczek <[email protected]> * Allow tp_overlap tree substitution in hydra config Signed-off-by: Jan Baczek <[email protected]> * Add warning in case of usage of the default tp config Signed-off-by: Jan Baczek <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Signed-off-by: Jan Baczek <[email protected]> * Change warning message Signed-off-by: Jan Baczek <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Signed-off-by: Jan Baczek <[email protected]> * Add compute capability resolver Signed-off-by: Jan Baczek <[email protected]> * Bugfix Signed-off-by: Jan Baczek <[email protected]> * Fix cherry pick Signed-off-by: Jan Baczek <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Signed-off-by: Jan Baczek <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Pass tp config via hydra Signed-off-by: Jan Baczek <[email protected]> * Remove self.ub_cfgs field - it isn't used anywhere else Signed-off-by: Jan Baczek <[email protected]> * Allow tp_overlap tree substitution in hydra config Signed-off-by: Jan Baczek <[email protected]> * Add warning in case of usage of the default tp config * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Change warning message Signed-off-by: Jan Baczek <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Add compute capability resolver Signed-off-by: Jan Baczek <[email protected]> * Bugfix Signed-off-by: Jan Baczek <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add guards to pynvml import Signed-off-by: Jan Baczek <[email protected]> --------- Signed-off-by: Jan Baczek <[email protected]> Signed-off-by: yaoyu-33 <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: yaoyu-33 <[email protected]>

github-actions bot added the NLP label Jul 10, 2023

jbaczek added 3 commits July 17, 2023 17:20

Pass tp config via hydra

edd95a0

Signed-off-by: Jan Baczek <[email protected]>

Remove self.ub_cfgs field - it isn't used anywhere else

2c192b0

Signed-off-by: Jan Baczek <[email protected]>

Allow tp_overlap tree substitution in hydra config

bdbc6bf

Signed-off-by: Jan Baczek <[email protected]>

jbaczek force-pushed the tp_conf_rework branch from 976277b to bdbc6bf Compare July 17, 2023 15:21

erhoo82 mentioned this pull request Jul 18, 2023

Draft: Move UB config files to NeMo NVIDIA/NeMo-Framework-Launcher#95

Closed

jbaczek and others added 4 commits July 19, 2023 10:36

Add warning in case of usage of the default tp config

d9f21a0

[pre-commit.ci] auto fixes from pre-commit.com hooks

e0ffd31

for more information, see https://pre-commit.ci

Change warning message

ca494a5

Signed-off-by: Jan Baczek <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

00fb374

for more information, see https://pre-commit.ci

erhoo82 closed this Jul 24, 2023

erhoo82 reopened this Jul 24, 2023

erhoo82 requested review from ericharper and erhoo82 July 24, 2023 16:19

erhoo82 previously approved these changes Jul 24, 2023

View reviewed changes

nemo/collections/nlp/models/language_modeling/megatron_gpt_model.py Outdated Show resolved Hide resolved

Add compute capability resolver

981a9a7

Signed-off-by: Jan Baczek <[email protected]>

jbaczek dismissed erhoo82’s stale review via 981a9a7 August 10, 2023 16:26

github-actions bot added the core Changes to NeMo Core label Aug 10, 2023

jbaczek and others added 3 commits August 11, 2023 11:22

Bugfix

c711b13

Signed-off-by: Jan Baczek <[email protected]>

Merge branch 'main' into tp_conf_rework

c286ec9

Signed-off-by: yaoyu-33 <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

d3d5e10

for more information, see https://pre-commit.ci

ericharper requested a review from titu1994 August 11, 2023 17:50

add guards to pynvml import

365e641

Signed-off-by: Jan Baczek <[email protected]>

titu1994 approved these changes Aug 11, 2023

View reviewed changes

ericharper approved these changes Aug 11, 2023

View reviewed changes

ericharper merged commit 2b9c5f4 into NVIDIA:main Aug 12, 2023

jbaczek mentioned this pull request Nov 23, 2023

Added YAML parsing for UB/TP overlap configs #7931

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load ub_cfg from hydra config #7003

Load ub_cfg from hydra config #7003

jbaczek commented Jul 10, 2023 •

edited

Loading

jbaczek commented Jul 17, 2023

erhoo82 commented Jul 24, 2023

erhoo82 left a comment

erhoo82 commented Jul 24, 2023 •

edited

Loading

erhoo82 commented Jul 31, 2023

jbaczek commented Jul 31, 2023

titu1994 left a comment

ericharper left a comment

Load ub_cfg from hydra config #7003

Load ub_cfg from hydra config #7003

Conversation

jbaczek commented Jul 10, 2023 • edited Loading

What does this PR do ?

Changelog

Usage

Before your PR is "Ready for review"

Who can review?

Additional Information

jbaczek commented Jul 17, 2023

erhoo82 commented Jul 24, 2023

erhoo82 left a comment

Choose a reason for hiding this comment

erhoo82 commented Jul 24, 2023 • edited Loading

erhoo82 commented Jul 31, 2023

jbaczek commented Jul 31, 2023

titu1994 left a comment

Choose a reason for hiding this comment

ericharper left a comment

Choose a reason for hiding this comment

jbaczek commented Jul 10, 2023 •

edited

Loading

erhoo82 commented Jul 24, 2023 •

edited

Loading