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 SDK version warning to AI tutorial #1433

Closed
wants to merge 1 commit into from

Conversation

mikkelhegn
Copy link
Member

Adding a small disclaimer that code samples in the article are for older SDK versions.

@mikkelhegn mikkelhegn requested a review from itowlson December 2, 2024 09:15
@mikkelhegn
Copy link
Member Author

@@ -42,6 +42,8 @@ In this tutorial we will:

## Tutorial Prerequisites

> Please note that the code sample in this article is not using the latest version of the Spin SDK, but works with the latest version of Spin (3.0.0). Please refer to the code in this [GitHub repository for the working samples and corresponding SDK version](https://github.com/fermyon/ai-examples/tree/main).
Copy link
Contributor

@karthik2804 karthik2804 Dec 2, 2024

Choose a reason for hiding this comment

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

I do not think we made any breaking changes regarding the SDK as far as llm is considered in a long time, and things should work, which confuses me.

Copy link
Member Author

@mikkelhegn mikkelhegn Dec 2, 2024

Choose a reason for hiding this comment

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

I tried with the Rust sample and I had to change it to make it work.

This is the diff I ended up with to make the Rust sample work:

5c5
<     http::{IntoResponse, Params, Request, Response, Router},
---
>     http::{IntoResponse, Json, Params, Request, Response, Router},
56c56,59
< fn perform_sentiment_analysis(req: Request, _params: Params) -> Result<impl IntoResponse> {
---
> fn perform_sentiment_analysis(
>     req: http::Request<Json<SentimentAnalysisRequest>>,
>     _params: Params,
> ) -> Result<impl IntoResponse> {
58,60c61
<     let sentiment_analysis_request: SentimentAnalysisRequest =
<         serde_json::from_slice(req.body()).expect("Failed to deserialize request body");
<     let sentence = sentiment_analysis_request.sentence.trim();
---
>     let sentence = req.body().sentence.trim();

Copy link
Member Author

Choose a reason for hiding this comment

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

Error with the orginal code

├╴E failed to resolve: use of undeclared crate or module `http`
 │   use of undeclared crate or module `http` rustc (E0433) [57, 10]
`

Never templates don't have 'http' as a dependency - can that be it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well I guess this has always been a problem? Cannot find any traces we had http in the template previously.

Copy link
Contributor

Choose a reason for hiding this comment

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

I vaguely recall us re-exporting http through the SDK (can definitely be misremembering).

Copy link
Member Author

Choose a reason for hiding this comment

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

The sample in the repo, which works without any changes use a fairly old SDK 1.5.0. I haven't checked if the other languages work based on new SDK version and "old" code sample.

Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

Could we not update the code rather than telling people to go back to an old SDK? Leading a tutorial with "this no longer works" bit feels kinda awkward.

@mikkelhegn
Copy link
Member Author

Yes we could, but all the samples in the repo that is referred in the article would probably need updating, unless we're ok having the article and repo be out of sync: https://github.com/fermyon/ai-examples/tree/main

@mikkelhegn
Copy link
Member Author

I've changed strategies and is now updating article and samples. Will close this and open a new PR.

@mikkelhegn mikkelhegn closed this Dec 18, 2024
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.

3 participants