-
Notifications
You must be signed in to change notification settings - Fork 333
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
Added watsonx.ai generator #1058
base: main
Are you sure you want to change the base?
Conversation
Added watsonx generator
Black Format
Added more documentation.
DCO Assistant Lite bot All contributors have signed the DCO ✍️ ✅ |
I have read the DCO Document and I hereby sign the DCO |
recheck |
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.
@iamnotcj, thank you for the useful contribution this is a great first pass at adding support for a the IBM stack.
There are some newer patterns in gathering configuration values that I would like to see applied here, please take a look and offer thoughts or adjustments based on my comments.
The dependency concern may be the largest item to sort out. If there are strong reasons the dependencies are required the project can weight the value trade offs.
Note the current package requires the following which may support features garak
does not really need per se:
"pandas<2.2.0,>=0.24.2",
"certifi",
"tabulate",
"packaging",
"ibm-cos-sdk<2.14.0,>=2.12.0",
"importlib-metadata",
Published code would help with understanding the needs for these dependencies. It is possible my search simply missed some public location.
Hey @jmartin-tech, thank you for the feedback! For the dependency problem, I think I will be able to just use the "request" library instead of the SDK. I will go ahead and make all of the necessary edits you highlighted in the other comments as well and update the request when its ready 👍 |
Dropped the SDK for requests library, Added some tests
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.
Added version parameter Co-authored-by: Jeffrey Martin <[email protected]> Signed-off-by: iamnotcj <[email protected]>
Added version parameter Co-authored-by: Jeffrey Martin <[email protected]> Signed-off-by: iamnotcj <[email protected]>
Updated the default version variable and updated tests.
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.
Awesome, this looks great!
This is the generator for watsonx.ai shown a couple weeks ago. It passes all of the tests on my computer.
I updated the generator test to include a "project_id" variable as a requirement for watsonx too.
I also tested it with the test probes and it was able to pass as well. It should be noted that I had to append a null byte to some of the probes, since it appears sending an empty string to leads to an api error in watsonx.
Please let me know if there are any issues with this request. :-)