-
Notifications
You must be signed in to change notification settings - Fork 43
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
Set gen_kwargs['n'] dynamically in the simple pipelines #144
Conversation
in draft form so I can let CI run. I'm also not in love with this implementation, but I don't have any better ideas ... |
I think we're saying "the pipeline author needs to tell us they want --sdg-scale-factor to be applied to this block" (as per instructlab/instructlab#1570) Would |
Thanks, I like that name better. |
We need a way to allow `--num-instructions`, or in the future `--sdg-scale-factor`, to influence how many instructions we generate using the simple pipelines. The way to do this seems to be to set `n` to this value. Since this is a runtime parameter, and we only want to set it for `n` in certain cases, add a new value for gen_kwargs['n'] called `scaled` which is a hint to use the runtime parameter here. Closes instructlab#130 Signed-off-by: Russell Bryant <[email protected]>
I pushed an update to rename the value to |
results = [] | ||
for prompt in prompts: | ||
for _ in range(n): | ||
for _ in range(generate_args["n"]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not get_n()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work and make the code more clear, but in this particular code path, the value was already converted up several lines when it did
generate_args = self._gen_kwargs(**gen_kwargs)
Otherwise just after this when it calls client.completions.create()
, it would break on an invalid value of n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pro: always use self._get_n()
so nobody is trying to think of when to use it or not
con: technically not required here
shrug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I might tweak a bit when I rebase #138 which changes this anyway
I checked the e2e CI output and it looks correct |
…_actions/rojopolis/spellcheck-github-actions-0.43.0 Bump rojopolis/spellcheck-github-actions from 0.42.0 to 0.43.0
We need a way to allow
--num-instructions
to influence how manyinstructions we generate using the simple pipelines. The way to do
this seems to be to set
n
to this value. Since this is a runtimeparameter, and we only want to set it for
n
in certain cases, add anew value for gen_kwargs['n'] called
dynamic
which is a hint to usethe runtime parameter here.
Closes #130
Signed-off-by: Russell Bryant [email protected]