search

tensorflow / java

Java bindings for TensorFlow
Apache License 2.0
785 stars 193 forks source link

Build core-platform only on deploy #514

Closed karllessard closed 5 months ago

karllessard commented 5 months ago

Small PR to build tensorflow-core-platform only when the deploying profile is active (i.e. when building and pushing artifacts to Sonatype). This way, users can intuitively run mvn install locally and they won't get error that some artifacts are missing (right now the "patch" is to pass the -Djava.platform.host parameter to get rid of this error).

karllessard commented 5 months ago

Another option though is to play with the list of native dependencies in the POM of tensorflow-core-platform itself to include only the current platform when deploying profile is not active. This way, you can product a tensorflow-core-platform locally and depend on it in your local projects. Thoughts?

Craigacp commented 5 months ago

The only use of platform now is as a metaproject to pull together all of the different platforms' binaries, so having it occasionally only contain a single platform seems like it might cause more trouble. I think this solution is fine.

karllessard commented 5 months ago

Ah oops :D I just did and submitted the other option... I though you might still want to include "tensorflow-core-platform" as a dependency in your local projects, so you don't need to include individually -native and -api... I can revert it to the original option, sorry I didn't saw your comment before

karllessard commented 5 months ago

I still believe it is useful the new way, thinking of it... imagine you want to run locally some project that depends on tensorflow-core-platform that you don't own. But you are running on Linux Arm64 so you do a local build of TF-Java to be able to run it. If the build does not produce tensorflow-core-platform, it will continue to fail to run. Does that make sense?

Craigacp commented 5 months ago

Sure.

saudet commented 5 months ago

Deploying also happens for developers that work on Mac or Windows but that deploy to Linux machines. You're going to have users wondering why their deployments suddenly start failing. That's why I personally prefer having it download everything by default, because then we don't have to muck around with profiles that don't work in situations X or circumstances Y with tools Z, such as Gradle or sbt.

karllessard commented 5 months ago

I just realized that I can't add dependencies in the profile like I'm doing now, unless I'm unwrapping them in tensorflow-core-platform to make it a uberjar of native libraries (which could work too). There are other ways to do it as well, I'll push a new version soon.

I'm not sure to understand your point @saudet . Users will still be able to develop on Mac or Windows and deploy on Linux when using the artifacts we are distributing. Here we just impact the users building TF Java locally. So chances are that they just want to be able to run their project locally on an unsupported platform (unless they also plan to have their own custom TF Java distribution).

Basically I'm doing the same as we are doing when are setting -Djavacpp.platform.host, but I now want this behaviour to be the default so that a simple intuitive mvn install on a local machine works, without the need of specifying anything else.

saudet commented 5 months ago

Like I said, the big problem here is that there are no other tools than Maven that support profiles. You will receive a ton of hate mail from Gradle and sbt users. The question is: Is this alright with you? If the answer is yes, then by all means, please go ahead! :) I'm not debating anything here. I'm just stating facts. Even Maven itself does not recommend using profiles for anything. If there is something you do not understand please tell me what that is. Just repeating over and over exactly the same thing about why using profiles is better isn't going to help move the conversation forward.

karllessard commented 5 months ago

@Craigacp , I've pushed a new version, can you please take a look? I'm just making javacpp.platform.host the default behaviour.

Craigacp commented 5 months ago

Looks fine to me.

Craigacp commented 5 months ago

Like I said, the big problem here is that there are no other tools than Maven that support profiles. You will receive a ton of hate mail from Gradle and sbt users. The question is: Is this alright with you? If the answer is yes, then by all means, please go ahead! :) I'm not debating anything here. I'm just stating facts. Even Maven itself does not recommend using profiles for anything. If there is something you do not understand please tell me what that is. Just repeating over and over exactly the same thing about why using profiles is better isn't going to help move the conversation forward.

This is about how our build works, it doesn't affect downstream users of TF-Java using other build systems as they will pull in published or deployed artifacts. We're not going to migrate TF-Java to use gradle or sbt, so that's not relevant.

saudet commented 5 months ago

Yes this change will affect downstream users big time. When you're done not listening to me, a better solution that doesn't break everything for Gradle, sbt, etc would be to use os-maven-plugin: https://github.com/bytedeco/javacpp-presets/issues/846#issuecomment-1482762303

Craigacp commented 5 months ago

Why will it affect them? They will continue to depend on tensorflow-core-platform as before, which will pull in all the binaries that we've published for that version along with tensorflow-core-api.

saudet commented 5 months ago

No it won't! If you don't believe me, try it yourself