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

Add initial build workflow for CI using GitHub Actions #8

Merged
merged 1 commit into from Jan 2, 2020

Conversation

saudet
Copy link
Contributor

@saudet saudet commented Dec 20, 2019

It took me a while to determine which versions of everything work, to research all the flags needed to work around various issues, and otherwise get all the magical incantations just right, but with these updates GitHub Actions is pretty much working on Linux, Mac, and Windows including MKL and CUDA for CI purposes. I had to upgrade to TensorFlow 2.1.0-rc2 because 2.0.0 doesn't build correctly when using Visual Studio 2019 and that's the only version available on the hosted virtual machines. The result looks something like this on my fork: https://github.com/saudet/tensorflow-java/commit/4d7340eea7bd354b8da2283e61c276360852e290/checks?check_suite_id=377172023

The only remaining issue is the small size of the page file on Windows that makes MSVC run out of heap space: https://github.community/t5/GitHub-Actions/Error-quot-The-paging-file-is-too-small-for-this-operation-to/m-p/41713#M4723

I've tried to work around that by using options like --local_cpu_resources=1 and --local_ram_resources=2048 for Bazel, and /Zm10 for the C++ compiler, but it's not helping. We'll need to wait until they fix this, or if Google is going to give us beefier machines with GPUs and everything to run this on anyway, we might as well do this now. @ewilderj What's the status on that front?

In any case, after we figure out something for the credentials on Sonatype OSS, we can start uploading snapshots automatically from these builds. @sjamesr How should we proceed?

@sjamesr
Copy link
Contributor

sjamesr commented Dec 22, 2019 via email

@saudet
Copy link
Contributor Author

saudet commented Dec 23, 2019

@sjamesr My username on Sonatype OSS is also saudet. If you can give me permission to deploy for org.tensorflow that would be great. We just need to file a ticket for requests like that here: https://issues.sonatype.org/

To have GitHub Actions upload artifacts tough, we need to add the credentials with this:
https://help.github.com/en/actions/automating-your-workflow-with-github-actions/creating-and-using-encrypted-secrets
I can do that for my own fork, but not here. I guess I could test it on my fork with my own personal credentials, and when it gets merged here, you can add the ones you've been using until now in the settings there. Sounds good?

BTW, for OSS projects Sonatype lets us use their own repo for snapshots as well, so that should be OK for now anyway, but sure, if you have or will have another Nexus instance somewhere that would be more appropriate, we can use that easily.

@sjamesr
Copy link
Contributor

sjamesr commented Dec 27, 2019

@saudet I just filed https://issues.sonatype.org/browse/MVNCENTRAL-5426

@saudet
Copy link
Contributor Author

saudet commented Dec 28, 2019

@saudet I just filed https://issues.sonatype.org/browse/MVNCENTRAL-5426

Thanks! BTW, I don't have access to that issue. It looks like OSSRH and MVNCENTRAL are 2 different "projects"...

Anyway, do you think we should merge what we have here right away, or should we first wait until we get builds on Windows working and deployment going?

Copy link
Collaborator

@karllessard karllessard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we start distributing our artifacts on OSS, I would like to clarify one point with licensing:

In actual TF Java artifacts (i.e. those issued from the main TF repository), the native artifacts (e.g. libtensorflow_jni.jar) contains a LICENSE file (and a new THIRD_PARTY_TF_JNI_LICENSES since 1.15, I think). These licenses list copyright notices from different libraries used by TF core runtime.

When we migrated to our Java repo, I took a snapshot of the LICENSE file and copy it under tensorflow-core-api/src/main/resources. But that was not very useful since the licenses must be attached to the same artifact/folder than the native libraries we distribute (i.e. that jars with the os-arch classifier) and now it was ending in our Java jar (without classifier).

So I moved it to tensorflow-core-api directly for now and I guess that we need to add some Maven rules to include it in our classified/native jars. Nonetheless, some details remain obscure to me:

  • Where do come from the LICENSE file (and the new THIRD_PARTY_TF_JNI_LICENCES) found in the TF Java 1.15 artifacts?
  • How do we stay in sync with changes in those files?
  • Are the licenses the same for all supported OS?

@sjamesr ?

@saudet
Copy link
Contributor Author

saudet commented Jan 1, 2020

@karllessard You'll need to be more specific about what changes you're requesting. You could also make those changes directly on my fork. That's probably going to be easier.

@karllessard
Copy link
Collaborator

@saudet Actually I don't request any changes, I just did it to prevent the PR to be updated for deployment and merged before we address this issue (which seems to be the organic evolution of your previous discussions :) )

Ok so what about we merge this one as is and make another PR for deploying the artifacts, once the credentials are set and licenses are fixed @sjamesr ?

@saudet
Copy link
Contributor Author

saudet commented Jan 2, 2020

I see, so that would be an additional issue that needs to be resolved before we can start deploying snapshots. Since running the builds and tests is still useful by itself, instead of trying to address all these issues here, it might make more sense to merge this PR right away. Actually, we should probably follow a policy of not trying to address multiple issues like that so PRs don't become stale and end up with merge conflicts all the time.

@karllessard
Copy link
Collaborator

I agree, I was just waiting for James to merge it since he started to review it but I guess he might be off for holidays so to speed up things, I'll do it right away, I think everyone is on the same page here.

@karllessard karllessard merged commit 517285c into tensorflow:master Jan 2, 2020
@karllessard
Copy link
Collaborator

Created this issue to follow up on licensing issue that (might) prevent us to enable automatic deployments: #10

deansher pushed a commit to deansher/java that referenced this pull request Mar 3, 2021
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.

None yet

3 participants