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

feat: added Google GenAI client; simplified IAI/clients API surface. #829

Merged
merged 5 commits into from
Jan 5, 2024

Conversation

bwplotka
Copy link
Contributor

@bwplotka bwplotka commented Jan 4, 2024

πŸ“‘ Description

Disclaimer: I am not a GenAI experts, or official Google delegate around those topics. I know what you know from public docs (: I am simply contributing some improvements here as I experiment with the OSS tooling around analyzing Kubernetes problems. Thought I could improve some CNCF software on the way.

This PR adds the newest https://ai.google.dev/ API (for Gemini and PaLM (legacy) models).

I think it might be fixing #799, since the Gemini API added here is also supported by Google Cloud Vertex AI but there is no Vertex AI client/API per se (AFAIK), there was only PaLM APIs, so not sure. Sorry @swastik959 if I accidentally solved the issue you were assigned to. I had to do this work anyway for my experiments.

On the way I couldn't understand why IAI interface is so complex. After investigation it seems it might be some tech debt from initial experiments, I proposed some changes to limit code duplication, improve readability and consistency (and fixed some gaps on the way). Specifically:

  • add auth flags specify which backend cares about which settings
  • AI clients are not expected to parse multiple "failure texts", process prompt template and cache. This is common behavior that might be better to be implemented once on Analysis function. This fixed numerous existing bugs with some providers/clients which had inconsistent logic for that (lack of caching/lack of processing/caching is not best effort etc). This was offloaded to one getAIResultForSanitizedFailures function. Thanks of that GenerateComplete is trvial and Parse can be removed.
  • AI clients are not aware of language. Current code only tries to force language with prompt prefix, so no need to inject it (we can inject later if needed). This simplifies Configure method.

βœ… Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

β„Ή Additional Information

Gemini API is still rolling out e.g. it's not available in UK yet πŸ™ˆ Full details: https://ai.google.dev/available_regions

Signed-off-by: bwplotka <bwplotka@gmail.com>
Signed-off-by: bwplotka <bwplotka@gmail.com>
@bwplotka bwplotka requested review from a team as code owners January 4, 2024 15:29
@bwplotka bwplotka changed the title feat+refactor: Added Google GenAI client; simplified IAI/clients API surface. feat: Added Google GenAI client; simplified IAI/clients API surface. Jan 4, 2024
Copy link

codecov bot commented Jan 4, 2024

Codecov Report

Attention: 42 lines in your changes are missing coverage. Please review.

Comparison is base (526e22f) 46.54% compared to head (8737cc4) 45.87%.

Files Patch % Lines
pkg/analysis/analysis.go 0.00% 40 Missing ⚠️
pkg/server/analyze.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #829      +/-   ##
==========================================
- Coverage   46.54%   45.87%   -0.67%     
==========================================
  Files          27       27              
  Lines        2198     2230      +32     
==========================================
  Hits         1023     1023              
- Misses       1137     1169      +32     
  Partials       38       38              

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

Copy link
Member

@arbreezy arbreezy left a comment

Choose a reason for hiding this comment

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

Great work thanks @bwplotka.

Lgtm although I can't test the new backend.
One minor thing, our semantic PR validation fails if you have a capital letter after the prefix.

@bwplotka bwplotka changed the title feat: Added Google GenAI client; simplified IAI/clients API surface. feat: added Google GenAI client; simplified IAI/clients API surface. Jan 4, 2024
@bwplotka
Copy link
Contributor Author

bwplotka commented Jan 4, 2024

Done, thanks!

Copy link
Contributor

@matthisholleville matthisholleville left a comment

Choose a reason for hiding this comment

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

Thank you @bwplotka for your contribution ! Good job ! Shouldn't we add an example in the readme? https://github.com/k8sgpt-ai/k8sgpt/blob/main/README.md#key-features

@bwplotka
Copy link
Contributor Author

bwplotka commented Jan 4, 2024

Yup, I can do that in later PR or this one, up to you. Tomorrow though, closing laptop for today!

Copy link
Contributor

@thschue thschue left a comment

Choose a reason for hiding this comment

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

lgtm

@thschue
Copy link
Contributor

thschue commented Jan 5, 2024

Yup, I can do that in later PR or this one, up to you. Tomorrow though, closing laptop for today!

Please open a new PR for the docs

Thank you for this contribution!

@thschue thschue merged commit e7d4149 into k8sgpt-ai:main Jan 5, 2024
7 checks passed
@bwplotka
Copy link
Contributor Author

bwplotka commented Jan 5, 2024

Thanks! Docs in progress πŸ’ͺ🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants