-
Notifications
You must be signed in to change notification settings - Fork 199
Add support for iOS targets #512
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
Add support for iOS targets #512
Conversation
It's ready for review now. |
wow,that's good. if we have multi engine in context(like okhttp),does we have some api to choose it? |
There is the engineFactory function for Ollama, but for other things I think it chooses in what's available (it works with Js engine already so it should work the same with other platforms) |
I also made a PR in |
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.
Please add only the following ios targets: iosArm64, iosX64, iosSimulatorArm64.
Also, have you tested it locally with ios?
Do we need to change the workflow for ios?
The more target we support, the more users can use the library everywhere. I always try to support as much targets as possible to be the more KMP possible. Also, there is not much work since Ktor does all the work of multiplatform abstraction, so we maintain only common code. |
Imagine I want to build a Desktop Native Windows app. Why shouldn't I be allowed to use Koog? So it perfectly makes sense as a library to be available on as many platforms as possible. |
The latest version of Kotlin Logging was published with the Kotlin 2.2.0 compiler so it does not work. Do I rollback and skip some watchOS targets? Or do I upgrade the project to use Kotlin 2.2.0 too? |
@nathanfallet
That’s not entirely accurate, we rely on other dependencies as well. In addition, supporting more targets complicates the
In practice, all of these use cases are already covered by the JVM target. Also, Compose doesn’t support these native targets Another important point is that Kotlin officially treats Linux as Tier 2 and Windows as Tier 3 platforms. This means Kotlin does not test mingwX64 on CI, and we might encounter compiler issues when building Koog for Windows. To manage this risk, we would need to introduce a tiered support model within Koog as well. So that users have a clear understanding of what’s officially supported and what may be unstable. Otherwise, we’d tie our release stability to the state of Kotlin’s native targets, which could negatively impact our main JVM releases Therefore, I suggest that in this PR we proceed only with iOS support. For the other native targets, we can open a separate issue to discuss support levels, the process for adding and releasing them, and associated maintenance implications |
Okay, I'll comment out the other targets, so we can uncomment them later when we want to (but they will be ready) EDIT: I made the change. I kept the targets inside comments so we can easily get them again later when we want, as well as the xxxMain folders (appleMain is used for iOS, but other are unused for now but will be later so we don't have to rewrite it) |
I will take a look about adding tests for iOS (configuring GH Actions to run on macOS) |
@devcrocod What do you think now? Also, I launched ios tests on macOS, but I might need to move some tests from jvmTest to commonTest, because otherwise they won't run on iOS (but strange thing since js & wasmjs are already added but they don't have any tests 🤷♂️). At least I can confirm iOS is compiling correctly from them succeeding. |
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 think it’s better to clean up the code from other targets to avoid confusion for other users and contributors
The main question is about testing, I hope @aozherelyeva can help with that part
.github/workflows/checks.yml
Outdated
@@ -98,6 +98,10 @@ jobs: | |||
if: matrix.os != 'ubuntu-latest' | |||
run: ./gradlew jvmTest --continue | |||
|
|||
- name: iosX64Test with Gradle Wrapper |
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 think the iosSimulatorArm64
tests should also be added, right?
@aozherelyeva How will this affect our current testing?
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.
The only failing test is OpenAIEmbedderTest
, but I'm not sure whether it's a critical issue, as JVM target is running for it without a problem.
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'm not even sure we should run them for the iOS target. @nathanfallet WDYT?
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.
The only failing test is OpenAIEmbedderTest
In my opinion, this is an issue because the provider code has no platform-specific dependencies and should run on any target
I also checked that macos-latest refers to macOS 14 on arm64, so iosX64Test
won’t run on it. gradle will just skip it, since for arm devices it should be iosSimulatorArm64
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.
Hm, yes, I also see that iosX64Test
is being skipped... good point!
@nathanfallet could you take a look at the tests?
linuxMain { | ||
dependencies { | ||
api(libs.ktor.client.curl) | ||
} | ||
} | ||
|
||
mingwMain { | ||
dependencies { | ||
api(libs.ktor.client.winhttp) | ||
} | ||
} |
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.
need to remove all such places
It’s strange that this doesn’t lead to an exception
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.
If use native/
, it will cover all cases with Kotlin/Native
Or do you think we’ll need to split it into specific targets in the future?
// Tiers are in accordance with <https://kotlinlang.org/docs/native-target-support.html> | ||
// Tier 1 | ||
//macosX64() | ||
//macosArm64() | ||
iosSimulatorArm64() | ||
iosX64() | ||
|
||
// Tier 2 | ||
//linuxX64() | ||
//linuxArm64() | ||
//watchosSimulatorArm64() | ||
//watchosX64() | ||
//watchosArm32() | ||
//watchosArm64() | ||
//tvosSimulatorArm64() | ||
//tvosX64() | ||
//tvosArm64() | ||
iosArm64() | ||
|
||
// Tier 3 | ||
//mingwX64() | ||
//watchosDeviceArm64() |
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 don’t think leaving commented-out code is a good practice
At the very least, adding the targets to the build script in the future won’t be a difficult task
linuxMain { | ||
dependencies { | ||
api(libs.ktor.client.curl) | ||
} | ||
} | ||
|
||
mingwMain { | ||
dependencies { | ||
api(libs.ktor.client.winhttp) | ||
} | ||
} |
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.
please drop it
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.
It would be good to get rid of the explicit/actual here, since ktor can choose the required engine automatically
However, if you decide to keep it, you’ll also need to remove it for linux and mingw, or place it under native/
@aozherelyeva @devcrocod I think we're good! I only kept changes related to iOS (linux and mingw changes were totally removed) and I also updated CI to run iOS tests for arm instead of previous ones. |
Wait, |
I found the problem: @aozherelyeva could you please advise? |
Thanks to @devcrocod, the problem is found and already solved. So now the |
b8b3546
to
600d415
Compare
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.
LGTM test-wise!
I guess it's good now. So excited for the multiplatform area to start! |
600d415
to
9a604fe
Compare
9a604fe
to
735d298
Compare
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.
finally 🎉
Congrats! 🚀 🔥 |
Congratulations!🎆🎆🎆 |
Hi! I can't build Koog on my Mac with these changes. I'm building from 8b313b0. Here's the build error:
Might be some issue on my side, but I thought it was worth sharing. |
@nicolasf Your error mentions 0.3.0.1 which is not publicly available (you can find it on https://packages.jetbrains.team/maven/p/grazi/grazie-platform-public/ai/koog/koog-agents/ but you need to add the repository to your gradle settings). But I don't get how it is related to the iOS target. |
@nathanfallet
The error will also happen without the env vars, but for 0.3.0 instead. If no one else has issues building it, it might be something on my environment. Thanks. |
have the iOS targets been published to maven central? |
I added iOS native targets
Fixes #177
Type of the change
Checklist for all pull requests
develop
as the base branchAdditional steps for pull requests adding a new feature