Skip to content

AI patch summary MVP #18

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

Open
JelteF opened this issue Jan 31, 2025 · 5 comments
Open

AI patch summary MVP #18

JelteF opened this issue Jan 31, 2025 · 5 comments

Comments

@JelteF
Copy link
Collaborator

JelteF commented Jan 31, 2025

One thing AI is actually very good at these days is reading a bunch of text very fast and producing a pretty useful (and mostly correct) summary. One specific type of summary this could give is what the blockers on a current patchset are and what are the disagreements. Such things for a very long email thread.

Sadly doing this automatically is no small task for at least a few reasons (and probably more):

  1. Who pays for the AI stuff?
  2. When is it run?
  3. Which AI tool to use?
  4. Most AI tools cannot read contents of links that you provide it
  5. The commitfest app does not actually store any data.
  6. There are hard limits on the amount of text you can give to an AI to summarize.

So instead of creating a solution that's fully integrated in the commitfest app, it seems better to start with an MVP that allows us to evaluate the usefulness of such summaries.

An MVP that should be relatively simple to implement is having a button on the patch page that populates the clipboard with a prompt that people can copy paste in their favorite AI tool. This would work around the first three issues. To solve the remaining issues you'd have to:

  1. Include the actual email thread text (and maybe patch text)
  2. Allow limiting the text in the prompt to a certain character limit (probably with a default that's allowed for the most popular AI tools)
@destrex271
Copy link
Contributor

destrex271 commented Feb 14, 2025

This will be a super helpful feature!!

Which AI tool to use?

I have two temporary/placeholder solutions to get started with this MVP:

  • We can utilize the Google Gemini API to spin up a quick MVP
  • We can use lightweight models from ollama like 'phi-3' and maybe setup a test instance for this? This will be a little harder to setup but might give us some pointers as to how we can host these models better

Most AI tools cannot read contents of links that you provide it

We have the content on our mail archives, so maybe we can either use web scraping or expose an endpoint from the archives website(or from wherever we get the email data) and feed that data to our model

There are hard limits on the amount of text you can give to an AI to summarize.

For the MVP I think we should focus on why the patch was required and who is involved with it. For example: Patch A was written because of bug x, and these people are involved in this. We can expand on the other components like what, when etc.. later.
Correct me if I am wrong, but I think we can get this information from first 1-2 emails in the threads associated with a patch for now.
Also maybe we can limit the scope of this feature to patches tagged as Ready for Commiter(or any other category that might help us in evaluating the usefulness of this feature)

If this seems reasonable, I can start working on small prototype with some dummy data.

@JelteF
Copy link
Collaborator Author

JelteF commented Feb 15, 2025

This will be a super helpful feature!!

Which AI tool to use?

I have two temporary/placeholder solutions to get started with this MVP:

* We can utilize the Google Gemini API to spin up a quick MVP

* We can use lightweight models from ollama like 'phi-3' and maybe setup a test instance for this? This will be a little harder to setup but might give us some pointers as to how we can host these models better

I definitely think that we'll eventually want do something like this. But the first option means we need to set up an account and figure out who pays for it. The second one means significant infrastructure work. As explained in my initial description, I think for an MVP we could skip these and only have a "populate clipboard with AI summary prompt" button. But if you think option 1 (gemini API) is not a lot of work to set up, feel free to include that too. As long as it's optional (based on an API key being being present in the settings) I think that's fine.

Most AI tools cannot read contents of links that you provide it

We have the content on our mail archives, so maybe we can either use web scraping or expose an endpoint from the archives website(or from wherever we get the email data) and feed that data to our model

I think scraping is probably the easiest. You can go to a whole thread by going to: https://www.postgresql.org/message-id/flat/

There are hard limits on the amount of text you can give to an AI to summarize.

For the MVP I think we should focus on why the patch was required and who is involved with it. For example: Patch A was written because of bug x, and these people are involved in this. We can expand on the other components like what, when etc.. later. Correct me if I am wrong, but I think we can get this information from first 1-2 emails in the threads associated with a patch for now. Also maybe we can limit the scope of this feature to patches tagged as Ready for Commiter(or any other category that might help us in evaluating the usefulness of this feature)

If this seems reasonable, I can start working on small prototype with some dummy data.

I'd definitely want to have it include some of most recent emails too. Threads tend to change focus a lot over time. And I think one of the most useful things is to figure out the current status of the thread, and what it's blocked on.

@destrex271
Copy link
Contributor

I'd definitely want to have it include some of most recent emails too. Threads tend to change focus a lot over time. And I think one of the most useful things is to figure out the current status of the thread, and what it's blocked on.

Yes this information is much more useful.

But if you think option 1 (gemini API) is not a lot of work to set up, feel free to include that too.

It's not a lot of work to setup but I'll need to use the generative-ai(https://pypi.org/project/google-generativeai/) sdk by google to access Gemini.(I can just send http requests, but that will be a lot of stuff to setup)

For now I think we can experiment with the "populate clipboard with AI summary prompt" button option for threads with relatively less number of threads.

I'll try to set it up on a different branch in my repository asap and maybe we can see how it works out.

Once we have this working, we can try sending it to Gemini and see what it generates.

@Florents-Tselai
Copy link
Contributor

Btw, I played with a version of this during pgconf.dev actually;

It's not too bad :)
Here are some examples

https://github.com/Florents-Tselai/pghaicker/tree/main?tab=readme-ov-file#examples

@Florents-Tselai
Copy link
Contributor

Latest example for https://www.postgresql.org/message-id/flat/aEC8HGy2tRQjZg_8@nathan

pghaicker summarize "aEC8HGy2tRQjZg_8@nathan"

Returns


This thread discusses a patch to add a GetNamedDSMHash() function to PostgreSQL, aiming to simplify the creation and attachment of hash tables in the DSM (Dynamic Shared Memory) registry.

Summary of Discussion:

  • Initial Proposal: Nathan Bossart proposes a patch (v1) that adds GetNamedDSMHash() to ease the creation/attaching of hash tables in the DSM registry.
  • Length of name: Dagfinn Ilmari Mannsåker raises a concern about using a hardcoded length (68) for tranche names instead of NAMEDATALEN.
  • Code Style and Correctness: Sami Imseih provides feedback on the structure of the NamedDSMHashState struct, suggesting better grouping of DSA and DSH related members for improved readability. He also recommends adding an Assert(dsh) check before returning the dshash table for safety. Sami later identified a potential issue with the suffixing of wait_event names, leading to user-facing strings like "pg_stat_statements_dsh". This is due to how tranche names are resolved and reported by pg_stat_activity. Sami also points out minor issues with the comment "if needed" being unnecessary and the found variable usage being potentially confusing.
  • Nathan's Responses and Patch Updates: Nathan responds to the feedback, incorporating the suggestions in subsequent patch versions (v2, v3, v4, and v5).
    • v2 addressed the naming issue, macro-izing the lengths and suffixes.
    • v3 included a comment in GetNamedDSMHash explaining the suffixing behavior.
    • v4 initializes the segment to avoid assumptions about zeroed-out memory.
    • v5 corrected a typo and increased the maximum name length from 64 to 128.
  • Wait Event Naming Issue: Sami raises concerns that the wait_event name in pg_stat_activity shows a "_dsh" suffix when the dynamic hash table tranche is registered. If the table is not registered in a particular backend, it reports extension instead, potentially showing two different event names for the same underlying lock. He proposes that extensions should be able to register tranches when loaded to avoid inconsistencies.
  • LWLock Tranche Registration: Nathan acknowledges that the current LWLock tranche registration mechanism requires each backend to register them individually, which may be outdated given the introduction of DSAs. He proposes a new approach where a shared LWLockTrancheNames table is used, eliminating the need for individual backend registration.
  • dsa_allocate call: Florents Tselai asks how to dsa_allocate in the same area as the returned dshash_table, pointing out that the dsa_handle is not returned.

Decision Points:

  1. Suffixing Wait Event Names: Decide whether to keep the "_dsh" suffix in wait event names or adopt a different naming convention. This requires an understanding of the implications for users and the existing naming conventions for built-in tranches.
  2. LWLock Tranche Registration Mechanism: Consider adopting a shared LWLockTrancheNames table to avoid inconsistent wait_event reporting. This might be a larger undertaking and could be addressed in a separate patch.
  3. dsa_allocate call: Decide whether to expose the dsa_handle to allow further memory allocation in the shared memory segment.
  4. DSMR_NAME_LEN: Decide whether the name length should be decoupled from NAMEDATALEN and whether it should be increased.

Blockers:

  1. LWLock Tranche Registration: The inconsistent wait_event reporting issue may need to be resolved before the patch can be committed. It might be decided to address this in a separate thread/patch if considered too complex.
  2. Consensus on Wait Event Naming: Agreement needs to be reached on the naming convention for wait events, especially regarding the "_dsh" suffix.
  3. Lack of Further Review: The patch needs more reviews and testing from other developers to ensure correctness and performance.

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

No branches or pull requests

3 participants