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

Prompt Learning Inference Improvements #4566

Merged
merged 17 commits into from
Aug 4, 2022

Conversation

vadam5
Copy link
Contributor

@vadam5 vadam5 commented Jul 19, 2022

Signed-off-by: Virginia Adams vadams@nvidia.com

Separates the gpt prompt learning eval script from the gpt model eval script to simplify inference script files and match t5 prompt learning inference workflow.

Collection: NLP

Changelog

  • Adds megatron_gpt_prompt_learning_eval.py script
  • Adds prompt learning inference config
  • Updated megatron_gpt_eval.py script
  • Updates gpt inference config
  • Updated gpt prompt learning dataset class
  • Updated gpt prompt learning model class generate and predict step methods

Usage

python megatron_gpt_prompt_learning_eval.py \
	virtual_prompt_model_file=models/squad_p_tuning_126m_tokens100_epochs100_layers4_1e-4.nemo \
        gpt_model_file=models/gpt_126m.nemo \
        inference.greedy=True \
        inference.add_BOS=False \
        trainer.devices=1 \
        tensor_model_parallel_size=1 \
        data_paths=["data/squad_short_test.jsonl"]

python megatron_gpt_prompt_learning_eval.py \
	virtual_prompt_model_file=models/squad_prompt_tuning_5b_tokens100_steps35000_2e-4.nemo \
        gpt_model_file=models/gpt_5b.nemo \
        inference.greedy=True \
        inference.add_BOS=False \
        trainer.devices=2 \
        tensor_model_parallel_size=2 \
        data_paths=["data/squad_short_test.jsonl"]

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)

Signed-off-by: Virginia Adams <vadams@nvidia.com>
Signed-off-by: Virginia Adams <vadams@nvidia.com>
Copy link
Collaborator

@yidong72 yidong72 left a comment

Choose a reason for hiding this comment

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

some minor comments


for dataset in sentences:
for line in open(dataset, 'r', encoding='utf-8').readlines():
self.sentences.append(line)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you still load all the sentences from the files into the memory. Why it avoids OOM?
Also consider add the doc string for RequestDataSet. the sentences can be multiple things now.

Copy link
Contributor Author

@vadam5 vadam5 Jul 25, 2022

Choose a reason for hiding this comment

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

Ended up making a separate prompt learning eval script and removing the use of request dataset for prompt learning inference all together.

Previously, the inference code was trying to load and run inference on the entire test set at once without breaking it into batches. This was causing OOM for large test sets. This change was to make sure the data was loaded and passed to the model in batches. Though, like I said above, I ended up removing this part all together.

Signed-off-by: Virginia Adams <vadams@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jul 20, 2022

This pull request introduces 2 alerts when merging 3482a32 into fea3775 - view on LGTM.com

new alerts:

  • 2 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Jul 25, 2022

This pull request introduces 2 alerts when merging d74d42c into 6b9617d - view on LGTM.com

new alerts:

  • 2 for Unused import

Signed-off-by: Virginia Adams <vadams@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jul 25, 2022

This pull request introduces 2 alerts when merging a1f342c into 6b9617d - view on LGTM.com

new alerts:

  • 2 for Unused import

@vadam5 vadam5 changed the title Improved prompt learning inference to avoid OOM Prompt Learning Inference Improvements Jul 26, 2022
Signed-off-by: Virginia Adams <vadams@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jul 26, 2022

This pull request introduces 3 alerts when merging ccbe930 into 793cf48 - view on LGTM.com

new alerts:

  • 3 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Aug 1, 2022

This pull request introduces 3 alerts when merging 150ceab into 1a9daa5 - view on LGTM.com

new alerts:

  • 3 for Unused import

Signed-off-by: Virginia Adams <vadams@nvidia.com>
@vadam5 vadam5 requested a review from yidong72 August 1, 2022 21:59
ericharper
ericharper previously approved these changes Aug 2, 2022
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

@yidong72 yidong72 left a comment

Choose a reason for hiding this comment

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

some suggestion about api improvements

Signed-off-by: Virginia Adams <vadams@nvidia.com>
@vadam5 vadam5 requested a review from yidong72 August 3, 2022 00:01
Copy link
Collaborator

@yidong72 yidong72 left a comment

Choose a reason for hiding this comment

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

see my comment.

Signed-off-by: Virginia Adams <vadams@nvidia.com>
@vadam5 vadam5 requested a review from yidong72 August 3, 2022 18:07
@lgtm-com
Copy link

lgtm-com bot commented Aug 3, 2022

This pull request introduces 1 alert when merging 3f84478 into 08a623b - view on LGTM.com

new alerts:

  • 1 for Wrong name for an argument in a class instantiation

Signed-off-by: Virginia Adams <vadams@nvidia.com>
yidong72
yidong72 previously approved these changes Aug 3, 2022
Copy link
Collaborator

@yidong72 yidong72 left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Virginia Adams <vadams@nvidia.com>
@vadam5 vadam5 merged commit f39bc66 into main Aug 4, 2022
Davood-M pushed a commit to Davood-M/NeMo that referenced this pull request Aug 9, 2022
* Improved prompt learning inference to avoid OOM

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Python style fix

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Made two prompt learning eval script seprate

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* updated CI tests and documentation

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Removed request dataset from eval script

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Inference runs as expected

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* address comments

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Changed input format for generate method

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Updated documentation, change dataset vairable name

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Updated unit tests

Signed-off-by: Virginia Adams <vadams@nvidia.com>

Co-authored-by: Eric Harper <complex451@gmail.com>
Signed-off-by: David Mosallanezhad <dmosallanezh@nvidia.com>
@vadam5 vadam5 mentioned this pull request Aug 9, 2022
Jorjeous pushed a commit to Jorjeous/NeMo that referenced this pull request Aug 11, 2022
* Improved prompt learning inference to avoid OOM

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Python style fix

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Made two prompt learning eval script seprate

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* updated CI tests and documentation

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Removed request dataset from eval script

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Inference runs as expected

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* address comments

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Changed input format for generate method

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Updated documentation, change dataset vairable name

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Updated unit tests

Signed-off-by: Virginia Adams <vadams@nvidia.com>

Co-authored-by: Eric Harper <complex451@gmail.com>
Signed-off-by: George <zelenfr@ya.ru>
piraka9011 pushed a commit to piraka9011/NeMo that referenced this pull request Aug 25, 2022
* Improved prompt learning inference to avoid OOM

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Python style fix

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Made two prompt learning eval script seprate

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* updated CI tests and documentation

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Removed request dataset from eval script

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Inference runs as expected

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* address comments

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Changed input format for generate method

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Updated documentation, change dataset vairable name

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Updated unit tests

Signed-off-by: Virginia Adams <vadams@nvidia.com>

Co-authored-by: Eric Harper <complex451@gmail.com>
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@ericharper ericharper deleted the prompt_learning_inference_improvements branch September 20, 2022 23:55
hainan-xv pushed a commit to hainan-xv/NeMo that referenced this pull request Nov 29, 2022
* Improved prompt learning inference to avoid OOM

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Python style fix

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Made two prompt learning eval script seprate

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* updated CI tests and documentation

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Removed request dataset from eval script

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Inference runs as expected

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* address comments

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Changed input format for generate method

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Updated documentation, change dataset vairable name

Signed-off-by: Virginia Adams <vadams@nvidia.com>

* Updated unit tests

Signed-off-by: Virginia Adams <vadams@nvidia.com>

Co-authored-by: Eric Harper <complex451@gmail.com>
Signed-off-by: Hainan Xu <hainanx@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants