-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Sample App: Adding the knative function build for the sentiment analysis service #5904
Sample App: Adding the knative function build for the sentiment analysis service #5904
Conversation
✅ Deploy Preview for knative ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
code-samples/eventing/bookstore-sample-app/ML-sentiment-analysis/README.md
Outdated
Show resolved
Hide resolved
…coming cloudEvent
code-samples/eventing/bookstore-sample-app/ML-sentiment-analysis/README.md
Show resolved
Hide resolved
I agree, and since cold start is already something on the back of customer
mind while working with serverless, we should not add extra perception
Thank you,
-N
…On Wed, 20 Mar 2024 at 12:33, Calum Murray ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
code-samples/eventing/bookstore-sample-app/ML-sentiment-analysis/README.md
<#5904 (comment)>:
> + # Sleep for 3 seconds to simulate a long-running process
+ sleep(3)
I think since this is providing an example of how to build an app with
Knative it doesn't necessarily make sense to make it slower just to show
that some real world stuff could be slow, so my vote would be to remove it
(as long as everything still works without the sleep)
—
Reply to this email directly, view it on GitHub
<#5904 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AISZCFBLFXZ7CTS63FH7PULYZG25JAVCNFSM6AAAAABEUF5TMCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNBZGQ2DCMRYHA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
code-samples/eventing/bookstore-sample-app/ML-sentiment-analysis/README.md
Outdated
Show resolved
Hide resolved
knative/func#2241 has been resolved. In order to make func invoke work correctly, readers need to install latest knative function when available. (release 1.14) |
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Leo6Leo, pierDipi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…sis service (knative#5904) * Adding the knative function build for the sentiment analysis service * Update the sample code, so that the returned result is a cloudEvent * Update the sample code to give a specific event type to the response cloudEvent * Update the tutorial doc * Update the tutorial doc * Remove the docker registry info * Fix nit * Modify the return response type and how python function handle the incoming cloudEvent * Adding the explaination for serving * Unhide the alert box portion * Remove the intentional delay * Remove the duplicated line * Make the input as json instead of plaintext * Update the version of cloudEvent and update the deployment instruction * Update the tutorial to use the public URL instead of cluster-IP * Display the input text in the response
Fixes #5889
Proposed Changes