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

Remove deprecated arguments from TE's TransformerLayer #7917

Merged
merged 3 commits into from
Dec 12, 2023

Conversation

jbaczek
Copy link
Collaborator

@jbaczek jbaczek commented Nov 20, 2023

What does this PR do ?

Transformer engine doesn't use this arguments since v0.7 and now in v1.0 they removed them completely.

Collection: [Note which collection this PR will affect]
NLP

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
  • [x ] Bugfix
  • Documentation

Who can review?

@ericharper

@github-actions github-actions bot added the NLP label Nov 20, 2023
@aklife97
Copy link
Collaborator

the PR changes look fine but we might want to note that this only changes things for NeMo local GPT models, not m-core based ones which are the ones we majorly use now

@ericharper
Copy link
Collaborator

I think the main issue is that user are used to configuring this from the config. Now TE expects an environment variable to be set. Mcore makes sure that the env var and the config are consistent. Can NeMo set the env var automatically so that the user doesn't have to set the env var?

@aklife97
Copy link
Collaborator

yes, thats right. It would probably be better if we automatically set the TE env variable if someone passes it in the config

@aklife97
Copy link
Collaborator

Ideally, I would prefer if this env var setting happens in m-core since m-core also relies on arg passing format and is the place where we actually interface with TE and setup TE/local layers. But if we feel that it might need broader discussion it might make sense to add this in nemo for now so that it unblocks the issue?

@ShriyaPalsamudram
Copy link
Collaborator

@ericharper @aklife97 can you approve this for now in nemo so it unblocks?

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
Copy link
Collaborator

jenkins

1 similar comment
@ShriyaPalsamudram
Copy link
Collaborator

jenkins

@ericharper ericharper merged commit a19a073 into NVIDIA:main Dec 12, 2023
11 checks passed
pzelasko pushed a commit to pzelasko/NeMo that referenced this pull request Jan 3, 2024
Signed-off-by: Jan Baczek <[email protected]>
Co-authored-by: Eric Harper <[email protected]>
Signed-off-by: Piotr Żelasko <[email protected]>
ssh-meister pushed a commit to ssh-meister/NeMo that referenced this pull request Feb 15, 2024
Signed-off-by: Jan Baczek <[email protected]>
Co-authored-by: Eric Harper <[email protected]>
Signed-off-by: Sasha Meister <[email protected]>
rohitrango pushed a commit to rohitrango/NeMo that referenced this pull request Jun 25, 2024
Signed-off-by: Jan Baczek <[email protected]>
Co-authored-by: Eric Harper <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants