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

Kotlin API #165

Open
wants to merge 61 commits into
base: master
Choose a base branch
from
Open

Kotlin API #165

wants to merge 61 commits into from

Conversation

rnett
Copy link
Contributor

@rnett rnett commented Dec 7, 2020

This is the WIP pull request for the Kotlin API, as discussed here. Feedback is welcome.

Adding nullability annotations to the Java API will be done in a separate PR.

The Session API is a prototype, I plan on proposing some changes to the Java api (mostly having run return a Map-like class) that would make it doable as extensions rather than new classes.

I plan on experimenting with adding platform artifacts to tensorflow-core-kotlin, which is why there's a bunch of currently unused javacpp stuff in the pom (they would just depend on their java counterparts).

There's an example of a basic MNIST network in src/test/kotlin/org/tensorflow/Example.kt. I can't run it due to jnitensorflow in java.library.path errors, so I have no idea if it works, but it shows off the API.

I'm using explicit API mode and have enabled contracts since there's a lot of DSL-style methods that benefit from it.

I'm using ktlint for formatting with a 120 character line limit. It currently only checks format on compilation (instead of auto-formatting), I could go either way on this.

@saudet
Copy link
Contributor

saudet commented Dec 7, 2020

I plan on experimenting with adding platform artifacts to tensorflow-core-kotlin, which is why there's a bunch of currently unused javacpp stuff in the pom (they would just depend on their java counterparts).

Maven makes it really hard to hack something for native libraries like that, but if you do it exactly like in tensorflow-framework, it will work.

@rnett
Copy link
Contributor Author

rnett commented Dec 7, 2020

I plan on experimenting with adding platform artifacts to tensorflow-core-kotlin, which is why there's a bunch of currently unused javacpp stuff in the pom (they would just depend on their java counterparts).

Maven makes it really hard to hack something for native libraries like that, but if you do it exactly like in tensorflow-framework, it will work.

Unfortunate, but thanks for saving me the trouble of finding that out myself. Doesn't tensorflow-framework just provide the API layer and require a tensorflow-core-platform dependency?

@saudet
Copy link
Contributor

saudet commented Dec 7, 2020

Unfortunate, but thanks for saving me the trouble of finding that out myself. Doesn't tensorflow-framework just provide the API layer and require a tensorflow-core-platform dependency?

Yes, we're not copying all of tensorflow-core/pom.xml there... If you need to add/modify some profiles, it's probably a better idea to modify them there, or is there something preventing you from doing that?

@rnett
Copy link
Contributor Author

rnett commented Dec 7, 2020

Yes, we're not copying all of tensorflow-core/pom.xml there... If you need to add/modify some profiles, it's probably a better idea to modify them there, or is there something preventing you from doing that?

Yeah, I'll just depend on the api layer like framework. That's what it's doing currently, I just need to clean out the leftover code from copying the core pom.

@zaleslaw
Copy link
Contributor

zaleslaw commented Dec 7, 2020

@rnett It looks awesome, you add a few brilliant things like contract and with() usage and the proposed example with Dense layer look cool!

But I keep in mind that in future we should add changes in Java and Kotlin ops both for each enough big PR for consistency, and I propose a solution here:

  1. Don't merge this PR (it's in draft now, I see) before API will not stabilizing (0.3 or 0.4) version
  2. Wait for multiple recievers in Kotlin (I totally agree here with @rnett ) - we could get a lot of benefits from that
  3. Wait for Tensor API (Tensor types) finalization and add some tensor operations in Kotlin (I could help in addition or review here)
  4. Add more tests in Kotlin (maybe copied from Java tests) that guarantee us that we will get the same results from Java and Kotlin API

What do you think, @karllessard, it's a good topic for discussion and you drive API changes now

@rnett
Copy link
Contributor Author

rnett commented Dec 7, 2020

In theory, and practice as far as I've seen (i.e. adding the ones op), any Ops added to the Java API will be automatically generated. I don't see any reason you would add an Op to the Kotlin API, I'm not even sure if it would work. We may want to add something to ensure that the kotlin generation is run when tensorflow-core is compiled, but I'm not familiar enough with Maven to know if that's possible or how to do it. Moving tensorflow-core-kotlin into tensorflow-core might also help, as then build scripts can just run install on tensorflow-core.

Other API changes may cause issues, but only for ones with Kotlin apis built on top of them. Currently this is only Graph and EagerExecutionEnviroment (and Session, but that's a proof of concept more than an actual API), and I think they are pretty stable (and the APIs are just DSL methods). Remember, too, that you can always just use the Java API from Kotlin. It may not be idiomatic, but it will work. While there will be more in the future, I definitely want to limit Kotlin APIs to extensions as much as possible, new classes like in the Session API should be avoided. Any changes to the Java API would have to be pretty significant to cause issues, and deleting the old Kotlin APIs is always a valid strategy if whoever is changing the Java API doesn't want to write new Kotlin ones.

I definitely plan on waiting for the type system refactor. I'd like to generate the inline methods using DataType, I think it will be possible. There's a few more Java api changes I'd like to make and incorporate, like Session and indexing. Other than that, if there are any big fundamental API changes planned (type refactor scale), I agree it would be a good idea to wait on this until they are done.

I don't think we need to wait for multiple receivers, as it's not coming until March (and even that's a prototype) and it would be a purely additive change. In the mean time I could define them in an interface and have KotlinOps implement it. It would still be an easy refactor.

I am going to add some tests. There shouldn't be any issues, since it's codegen that calls the Java method directly, but I do want to add a few manual tests just to ensure that.

@rnett
Copy link
Contributor Author

rnett commented Dec 8, 2020

Also, while I think there's too many to change at this point, field() style getters are not replaced by properties in Kotlin the way getField() getters are. Given the how much nicer field() is for Java I'm not sure changing it is worth it. It would be possible to generate extension properties based on an annotation, but it doesn't seem worth it for a pair of parentheses.

@google-cla
Copy link

google-cla bot commented Dec 27, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

1 similar comment
@google-cla
Copy link

google-cla bot commented Dec 28, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@rnett
Copy link
Contributor Author

rnett commented Mar 20, 2021

@karllessard @Craigacp @zaleslaw (and anyone else w/ comments) This is ready for review. NonNull annotations will come in another PR, although I don't love how much they clutter up the Java code, so there may be some compromise on that.

Tests are currently failing because of #248.

@rnett rnett changed the title [WIP] Kotlin API Kotlin API Mar 20, 2021
@rnett
Copy link
Contributor Author

rnett commented Apr 23, 2021

After todays call: move to a tensorflow-kotlin that includes framework too.

Also, make the NdArray classes like Shape Kotlin-friendly (i.e. rename size to get) rather than adding extensions.

@rnett
Copy link
Contributor Author

rnett commented Apr 23, 2021

Ok, I've split the artifacts into tensorflow-core-kotlin, tensorflow-framework-kotlin, and tensorflow-kotlin which depends on the other two.

@rnett
Copy link
Contributor Author

rnett commented May 15, 2021

NDArray parts have been moved to tensorflow/java-ndarray#1, which should be merged first.

@rnett
Copy link
Contributor Author

rnett commented Jun 10, 2021

The more I play with KotlinOps the less I like that it is not a subclass of Ops. I'm not sure if there's a way around it, but I'm going to at least look into it.

@karllessard
Copy link
Collaborator

The more I play with KotlinOps the less I like that it is a subclass of Ops. I'm not sure if there's a way around it, but I'm going to at least look into it.

Keep in mind that it would be great if the solution for the Kotlin ops could also apply to the framework ops.

@rnett
Copy link
Contributor Author

rnett commented Jun 10, 2021

I'm not sure it would, since afaik framework ops is adding more ops, while the Kotlin ops need to override the Java ones. But I'll look.

@rnett
Copy link
Contributor Author

rnett commented Jun 10, 2021

There doesn't look to be a good solution. I'd use extensions, but importing them does not work well (see KTIJ-18859). If I get a quick response to that issue it might be feasible to just wait for a fix.

In the mean time, the best I came up with is:

public object KotlinExt {
    public fun <T : TType> Ops.gather(
        params: Operand<T>,
        indices: Operand<out TNumber>,
        axis: Operand<out TNumber>,
        batchDims: Long? = null
    ): Gather<T> = gather<T>(
        params,
        indices,
        axis,
        *listOfNotNull(
            batchDims?.let { org.tensorflow.op.core.Gather.batchDims(it) }
        ).toTypedArray()
    )
}

which lets you do something like:

private fun Ops.testActivation(x: Operand<TFloat32>): Operand<TFloat32> {
    with(KotlinExt) {
        gather(x, x, x, batchDims = 4)
        return nn.relu(x)
    }
}

with full autocomplete and type hints (and the extension functions are importable w/o the object).

With some helper functions it's not bad, and decorators would help significantly. Thoughts @karllessard @zaleslaw?

@rnett
Copy link
Contributor Author

rnett commented Jun 17, 2021

So in light of the context receivers proposal, I'm thinking of something like this in tensorflow-core-api:

public interface WithOps{
  public Ops getTf();
  public default WithOps withSubScope(String name) ...
}
public final class Ops implements WithOps {
  @Override
  public Ops getTf(){
    return this;
  }
}

This allows Kotlin code to use WithOps (potentially renamed WithTF or TfOps) as a context receiver without polluting the name space, while maintaining interoperability with Java classes ((WithOps) -> T is a subtype of (Ops) -> T, so you could pass any function defined w/ the Kotlin style to functions expecting a Java style lambda).

rnett and others added 29 commits December 21, 2021 18:01
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
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.

4 participants