google cloud run scoring implementation

[nkpng2k]

First attempt at scoring implementation that works for google cloud run, on customer request for scoring option in gcp.

@osery I tried to make this as minimalist as possible so there is almost no copy paste. I had to make a few things in the local rest scorer public in order to do so though, but I think nothing too scary.

basically the only difference between this and local rest scorer is that it tries to download mojo and license from gcs before starting

[osery]

Could we get away without the config file and the controller file? We could just do the downloading and call the local-rest-scorer's main. No need to have anything spring related. Wdyt? That way we don't risk people accidentally adding stuff here for new endpoints and such.

[nkpng2k]

@osery Thank you for the fast review. Made some changes as per your recommendations let me know what you think when you have the chance. Thanks!

[osery]

Thanks for all the changes @nkpng2k 🙏.
LGTM, only nits left.

I created a PR #157 which may be unneeded now. But still probably makes sense to have it...

[osery]

We should not need this, no? Should be brought in as the dependency already, I think 🤔.

[nkpng2k]

yes it technically is. Some weird issue with jib from what I could find... regarding this and the below comment. One of the jib issues I found while googling up on this: GoogleContainerTools/jib#954. But long story short is that jib doesn't use the springboot bootjar as one would expect, but takes the .class files created inside the build dir of the project. So the required local-rest-scorer .class files don't get added to the classpath and things don't work.

Side Note: we could use arg containerizingMode = 'packaged' which essentially places the actual boot jar into the classpath and we can then override the entrypoint, but it increases the size of the resultant docker image by nearly 30% so I figured this to be the less impactful method.

[osery]

I think we need to mark the bootJar in local-rest-scorer as archiveClassifier = "boot" so that the dependency in the gcp one actually brings in the plain jar as opposed to the bootJar (which has a special form and the ScorerApplication class is well hidden deep inside it).

I tried it removing the extraDirectories and extraClasspath and it seems to work.

I would personally rather use that than the packaged mode. Not pushing though.

More context: https://docs.spring.io/spring-boot/docs/current/gradle-plugin/reference/html/#packaging-executable-and-normal

[nkpng2k]

ahh this looks interesting. Let me try quickly. If it works I will push a change before the merge.

[nkpng2k]

@osery this worked great! can't believe I didn't notice this. Thanks! Made change to remove extraDirectories and extraClasspath values and added archiveClassifier = 'boot' to local-rest-scorer build.gradle

ran the docker image for local rest scorer and updated readme with correct jar for local run.

[osery]

LGTM, assuming the proposed fix work.

I have one more possibly late comment. Did you think about other providers? E.g., AWS?
Should we have some naming convention in the subprojects and possibly also the resulting docker images?

For example: local-rest-scorer, gcp-rest-scorer, aws-rest-scorer, ...? Wdyt? Or maybe even better rest-scorer-local, rest-scorer-gcp, ...

[osery]

I think we need to mark the bootJar in local-rest-scorer as archiveClassifier = "boot" so that the dependency in the gcp one actually brings in the plain jar as opposed to the bootJar (which has a special form and the ScorerApplication class is well hidden deep inside it).

I tried it removing the extraDirectories and extraClasspath and it seems to work.

I would personally rather use that than the packaged mode. Not pushing though.

More context: https://docs.spring.io/spring-boot/docs/current/gradle-plugin/reference/html/#packaging-executable-and-normal

[nkpng2k]

LGTM, assuming the proposed fix work.

I have one more possibly late comment. Did you think about other providers? E.g., AWS?
Should we have some naming convention in the subprojects and possibly also the resulting docker images?

For example: local-rest-scorer, gcp-rest-scorer, aws-rest-scorer, ...? Wdyt? Or maybe even better rest-scorer-local, rest-scorer-gcp, ...

I really like the above comment... let us open an issue for it and handle it separately. One thing to keep in mind though is that we should likely keep the product in the name somewhere. So I agree, but caveat: rest-scorer-local, rest-scorer-gcp-cloud-run, rest-scorer-aws-lambda, rest-scorer-aws-sagemaker. Either that or nest the projects to a dir structure:

dai-deployment-templates
|-- aws
    |-- lambda
    |-- sagemaker
|-- gcp
    |-- cloud-run
|-- local

wdyt?

[nkpng2k]

@osery just fyi, merged and opened a new issue #159 to track your above comment. Hope this is ok. We can address anything I missed in an additional pr. Sorry if merged prematurely...