-
Notifications
You must be signed in to change notification settings - Fork 37
added sdg api library design #98
Conversation
russellb
left a comment
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.
Some tips for the linter issues
make md-lint-fixwill fix most of the md-lint issues. I think there is one type of issue to fix manually after that.- and then for
make spellcheckyou have to add entries to.spellcheck-en-custom.txt
docs/sdg/sdg-api-interface.md
Outdated
| # model parameters for generation | ||
| model_name: mistralai/Mixtral-8x7B-Instruct-v0.1 |
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.
I think there's a need to configure the model name on a per-stage basis. We may have an endpoint configured with more than one model and we want to hit different ones depending on the stage.
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.
This was marked as resolved, but I don't think it got moved.
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.
@russellb made updates to this to accept a client, model-name and default model prompt
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.
@oindrillac but it's still not specified on a per-stage basis?
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.
the value will be a class variable that gets passed on when an object is initialized. we have also decided to keep the run config in a .py file as per your suggestion let me update the doc
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.
PTAL @russellb
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.
099d0b4 to
abc3b2b
Compare
russellb
left a comment
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 for the updates! There are just two minor comments left to resolve.
- moving
model_nameto be per-step - noting that
model_templateandstop_tokencan be optional and defaulted to the right thing for the subset of modelsilabsupports right now. - I suggested an edit to describe two stages of CLI integration:
- changing
generate_data()without impact to the CLI code - adding new options to the CLI to allow custom pipeline extensions
- changing
docs/sdg/sdg-api-interface.md
Outdated
| # model parameters for generation | ||
| model_name: mistralai/Mixtral-8x7B-Instruct-v0.1 |
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.
This was marked as resolved, but I don't think it got moved.
|
just checking back in on this -- there's still at least one unresolved comment. You should also squash all the commits back down to one. That will resolve the DCO error in CI. It looks like something else needs to get added to the spellcheck dictionary, as well. |
|
its breaking on |
|
Signed-off-by: Oindrilla Chatterjee <oc@bu.edu> Co-authored-by: Aakanksha Duggal <aduggal@redhat.com>
79fe0fd to
5517da7
Compare
|
#103 <- so that Spell Check passes on this PR |
for this PR to pass CI, that change needs to be included in this PR instead of a separate one. |
Co-authored-by: Sébastien Han <seb@redhat.com> Co-authored-by: Russell Bryant <rbryant@redhat.com> Signed-off-by: Oindrilla Chatterjee <oc@bu.edu>
|
done, added this here |
russellb
left a comment
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.
please see the open comment thread about the config format and model name
Signed-off-by: Oindrilla Chatterjee <oc@bu.edu>
russellb
left a comment
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.
thank you!
No description provided.