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: genai-rag vertex search (init) #5

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

feat: genai-rag vertex search (init) #5

wants to merge 33 commits into from

Conversation

glasnt
Copy link
Contributor

@glasnt glasnt commented Feb 14, 2025

Thank you for opening a Pull Request!

Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

@glasnt glasnt requested a review from a team as a code owner February 14, 2025 02:22
template {
template {
containers {
# Replace with ingestion job container
Copy link

Choose a reason for hiding this comment

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

is this a comment as a TODO or is this for the sample reader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the reader. While in devmode I've added TODO(glasnt) for notes for myself, rather than usage notes. Unsure how best to relay that aside from this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a few style choices that I'm making here, for now (next commit) I'm trying Note: above when things should be customized. I also want to mark Design Considerations in a way that's obvious as well. I'll at least be consistent in this sample, but unsure how far I want to denote repo-specific styleguides around this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

# See the License for the specific language governing permissions and
# limitations under the License.

# Aspects to setup ingestion from Cloud Storage bucket to processing job.
Copy link
Member

Choose a reason for hiding this comment

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

Don't we also need a "google_storage_notification" resource to enable notifications to be sent to the Pub/Sub topic when a new object is added or updated in the GCS bucket ?

https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/storage_notification#event_types-1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I would be handling this through ingestion_data_source_settings, but I'd have to check what the differences are here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've established that using a base of https://github.com/terraform-google-modules/terraform-docs-samples/blob/main/functions/pubsub/main.tf, adding https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/storage_notification#example-usage to put data into the topic, and adapting the JavaScript sample to Python (on request), I have a working ingestion pipeline

index = google_vertex_ai_index.default.id
dedicated_resources {
machine_spec {
machine_type = "n1-standard-16"
Copy link
Member

Choose a reason for hiding this comment

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

I recommend that we define a variable for this in variables.tf. Users might want to experiment with the machine type
to optimize index performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a lot of elements the user might want to change in vertex.tf to optimize performance, not just the deployed index, but all the index configurations. I'll use comments rather than variables to denote this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In c84256c I denoted a tentative comment scheme to differentiate between settings that are Design Considerations, and settings that should be adapted by the user.

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

Successfully merging this pull request may close these issues.

3 participants