-
Notifications
You must be signed in to change notification settings - Fork 139
feat: build provider image from GitHub #1236
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
Conversation
Summary of ChangesHello @jezekra1, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant new feature: the ability to build provider images directly within the BeeAI platform from GitHub repositories. This enhancement simplifies the process of deploying agents by automating the image creation step. It includes updates across the CLI for initiating builds, the SDK for interacting with the new build APIs, and the server-side infrastructure (Kubernetes and Helm charts) to manage the entire build lifecycle, from cloning code to pushing the final image to a local registry. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a significant new feature: server-side building of provider images from GitHub repositories. The changes span across the CLI, SDK, and server components, including new API endpoints, database models, Kubernetes job templates, and Helm chart configurations. The implementation is comprehensive, adding support for both public and private GitHub repositories and providing a good user experience through the CLI with log streaming. My review focuses on a few key areas to improve correctness and robustness. I've identified a critical issue in the SDK, a high-severity issue in the Kubernetes job template that would cause builds to hang, and a couple of medium-severity issues related to error handling and code clarity. Overall, this is a great addition to the platform.
| async def delete(self: ProviderBuild | str, *, client: PlatformClient | None = None) -> None: | ||
| # `self` has a weird type so that you can call both `instance.delete()` or `ProviderBuild.delete("123")` | ||
| provider_id = self if isinstance(self, str) else self.id | ||
| async with client or get_platform_client() as client: | ||
| _ = (await client.delete(f"/api/v1/provider_builds/{provider_id}")).raise_for_status() |
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.
There's a variable naming inconsistency in the delete method that could lead to confusion or bugs. The variable provider_id is being used to store a provider build ID. It should be renamed to provider_build_id for clarity and to match its actual purpose. This is especially important as it's used to construct the deletion URL.
| async def delete(self: ProviderBuild | str, *, client: PlatformClient | None = None) -> None: | |
| # `self` has a weird type so that you can call both `instance.delete()` or `ProviderBuild.delete("123")` | |
| provider_id = self if isinstance(self, str) else self.id | |
| async with client or get_platform_client() as client: | |
| _ = (await client.delete(f"/api/v1/provider_builds/{provider_id}")).raise_for_status() | |
| async def delete(self: ProviderBuild | str, *, client: PlatformClient | None = None) -> None: | |
| # `self` has a weird type so that you can call both `instance.delete()` or `ProviderBuild.delete("123")` | |
| provider_build_id = self if isinstance(self, str) else self.id | |
| async with client or get_platform_client() as client: | |
| _ = (await client.delete(f"/api/v1/provider_builds/{provider_build_id}")).raise_for_status() |
...-server/src/beeai_server/infrastructure/kubernetes/default_templates/build-provider-job.yaml
Show resolved
Hide resolved
apps/beeai-server/src/beeai_server/api/routes/provider_builds.py
Outdated
Show resolved
Hide resolved
8caf268 to
6cb731f
Compare
da56d52 to
44454d7
Compare
Signed-off-by: Radek Ježek <[email protected]>
44454d7 to
8fbc212
Compare
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Radek Ježek <[email protected]>
| # To change UID/GID, you need to rebuild the image | ||
| runAsUser: 1000 | ||
| runAsGroup: 1000 |
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 will be problematic, cluster might have a specific range of users/groups to run the container.
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 is taken directly from
https://github.com/moby/buildkit/blob/master/examples/kubernetes/job.rootless.yaml
there are not really many options :/ user namespaces seem to be a beta feature (likely disabled in existing clusters) and priviledged seems too risky
| ) | ||
|
|
||
|
|
||
| class SqlAlchemyProviderBuildRepository(IProviderBuildRepository): |
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.
Personally I'd try to use k8s to track the builds leveraging metadata unless I'd have a good reason not to but this is also fine.
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 chose to duplicate the status in database because we are auto-deleting the jobs after 10 minutes (otherwise we might run into maximum pod quotas)
a577734 to
15c8d5e
Compare
Signed-off-by: Radek Ježek <[email protected]>
15c8d5e to
1f6c273
Compare
Implements: #1160