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

Flattened inf params #152

Merged
merged 19 commits into from
Aug 31, 2023
Merged

Flattened inf params #152

merged 19 commits into from
Aug 31, 2023

Conversation

gkumbhat
Copy link
Collaborator

@gkumbhat gkumbhat commented Aug 29, 2023

Supports #155

@gkumbhat gkumbhat marked this pull request as ready for review August 29, 2023 19:57
Copy link
Collaborator

@alex-jw-brooks alex-jw-brooks left a comment

Choose a reason for hiding this comment

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

This looks great! Some initial thoughts, but mostly just discussion points

caikit_nlp/modules/text_generation/peft_prompt_tuning.py Outdated Show resolved Hide resolved
caikit_nlp/modules/text_generation/peft_prompt_tuning.py Outdated Show resolved Hide resolved
top_p: Optional[float] = 0.0,
typical_p: Optional[float] = 0.0,
temperature: Optional[float] = 1.0,
repetition_penalty: Optional[float] = 0.0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comments for repetition penalty and top_p here

truncation = True

if repetition_penalty == 0.0:
repetition_penalty = 1.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see now - but why is this being overridden here rather than just using 1 as the default in .run?

Is top_p supposed to be handled outside of generate like this too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

caikit_nlp/toolkit/text_generation/model_run_utils.py Outdated Show resolved Hide resolved
caikit_nlp/toolkit/text_generation/model_run_utils.py Outdated Show resolved Hide resolved
typical_p=0.23,
temperature=0.77,
)
assert isinstance(pred, GeneratedTextResult)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be nice to add some kind of validation to trace the input args to generate or something like that? Since these tests aren't actually validating that greedy / sampling etc are happening

error.type_check("<NLP84635843E>", int, allow_none=True, top_k=top_k)
error.type_check("<NLP55267523E>", float, allow_none=True, top_p=top_p)
error.type_check("<NLP13670202E>", float, allow_none=True, typical_p=typical_p)
error.type_check(
Copy link
Contributor

Choose a reason for hiding this comment

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

type check for decoding_method, temperature, max_time and exponential_decay_length_penalty? And a value check on decoding_method something like this?

Amount of time in seconds that the query should take maximum.
NOTE: this does not include network overhead.
Range: 0-120.0
exponential_decay_length_penalty: Tuple(int, float)
Copy link
Contributor

Choose a reason for hiding this comment

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

Type should also include ExponentialDecayLengthPenalty

@gkumbhat gkumbhat merged commit 4db0c26 into caikit:main Aug 31, 2023
4 checks passed
@gkumbhat gkumbhat deleted the flattened_inf_params branch August 31, 2023 17:35
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.

3 participants