Bug 2010707 - Created an LLMProvider class in Fenix

[fmasalha]

try attempt: https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=177275

[github-actions]

View this pull request in Lando to land it once approved.

[segunfamisa]

Thanks for this patch!

Most of my comments are not blocking, but I do have two comments that I think we should address:

1. About the creation of GeminiNanoLlm and
2. The check about the content being readerable

[segunfamisa]

💡 tip:

Do you know about this """my multiline text""".trimIndent()

I think it's pretty cool and can simplify long lines of text like this.

[segunfamisa]

⚠️ important:

@MatthewTighe : Do we also need to check if the page is readerable? i.e if the reader mode is enabled for that page.

How do we plan to check that?

One idea is:

1. Change the content parameter into a contentProperties of type data class ContentProperties(val length: Int, val isRreaderable: Boolean, .....)
2. Then we can just use contentProperties.length, llm.checkStatus() and contentProperties.isReaderable

[segunfamisa]

👌

[segunfamisa]

💡 idea:

Do we want to consider passing down the prompt from outside the summarizer?

So we can have:

class Summarizer(val llm: Llm, val prompt: String) {  
  
  suspend fun summarize(content: String) ... {
    val statement = "$prompt $content"
    return llm.prompt(Prompt(statement))
  }
}

or

class Summarizer(val llm: Llm) {  
  
  suspend fun summarize(content: String, prompt: String) ... {
    val statement = "$prompt $content"
    return llm.prompt(Prompt(statement))
  }
}

I like the second one better, because I can imagine that we want to A/B test multiple prompts with the same LLM, so we just call summarizer.summarize(..., promptA), summarizer.summarize(....,promptB)

[fmasalha]

try attempt: https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=177577

[fmasalha]

try attempt: https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=177582

[segunfamisa]

Thanks @fmasalha

There's one last thing I think we should fix before we land it. It's about how the test is written.

Let me know your thoughts.

[segunfamisa]

I think there's a recent effort from @mcarare to remove usages of MainCoroutineRule and runTestOnMain.

I recommend taking a look at one of Mihai's patches like this one https://phabricator.services.mozilla.com/D281798

In some cases, this is as simple as using runTest() instead of runTestMain() but if we are doing some special things with dispatcher changes, or delay or syncrhonization, it will require creating a testDispatcher = StandardTestDispatcher() and using runTest(testDispatcher) and advanceTimeUntilIdle()

But I don't think we are doing that in this case, so your test can just remove the test rule, and use runTest.

If that does not work. Lemme know, and we can work through what I described together.

[segunfamisa]

Is there any reason why this can't live in our feature module?

[fmasalha]

No reason, ill move it. thanks for the tip.