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

chore: bump spin version to 1.5.0 #151

Merged
merged 1 commit into from
Sep 21, 2023
Merged

Conversation

jprendes
Copy link
Contributor

Bump spin 1.4.1 -> 1.5.0.

@jprendes
Copy link
Contributor Author

@cpuguy83 @devigned PTAL

Copy link
Member

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

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

lgtm! But it looks like the CI failed

@Mossaka Mossaka closed this Sep 18, 2023
@Mossaka Mossaka reopened this Sep 18, 2023
@jprendes
Copy link
Contributor Author

IIUC, there's a problem building a dependency introduced by the new LLM feature of spin.

@Mossaka
Copy link
Member

Mossaka commented Sep 19, 2023

Is it becuase the spin image is still using spin SDK v1.4.1?

Nvm, I realized that it is the shim build that failed.

@jprendes
Copy link
Contributor Author

Ok, I digged a bit into it.
What's failing is building llama-cpp in aarch64 musl.
It builds fine with gnu ABI adding the -Ctarget-feature=+fp16 (what upstream spin does).
But it fails with musl ABI.
There are a few diferences, including the gcc version that cross uses for those targets.

The main issue is that llama-cpp is using the vld1q_u8_x4 and vld1q_s8_x4 functions, which should be SIMD intrinsics exposed by the compiler.

According to this issue xmrig/xmrig#2882, that intrinsic is not present in gcc <= 11.

I am wondering what the right fix is, since spin doesn't provide a feature flag to disable llm.

@jprendes
Copy link
Contributor Author

jprendes commented Sep 20, 2023

Ok, the solution I found is less than idea, and it's polyfilling the missing functions in the cross container.
I think this is an issue to be fixed upstream, this is just an ugly workaround.
I created fermyon/spin#1786

rust-toolchain.toml Show resolved Hide resolved
Copy link
Member

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

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

lgtm!

minor comment: add a readme on polyfill folder to describe what this does and when shall we remove it.

This can be done in a future PR.

@Mossaka
Copy link
Member

Mossaka commented Sep 21, 2023

Need a rebase

@jprendes jprendes force-pushed the bump-spin branch 2 times, most recently from 7145cfc to f636a57 Compare September 21, 2023 12:31
@jprendes
Copy link
Contributor Author

jprendes commented Sep 21, 2023

minor comment: add a readme on polyfill folder to describe what this does and when shall we remove it.

Done.

If there's an update to the compiler in cross-rs, the polyfill will become a no-op as it does a feature test before acting.
I left a link to the upstream issue in spin.
There are many different solutions, off the top of my head I can think of:

  • a feature gate in spin
  • a compiler update in cross
  • replacing cross with something like xx, similar to what we do in runwasi (although cross is really nice, but would also allow us to build with systemd and seccomp support here)

@Mossaka
Copy link
Member

Mossaka commented Sep 21, 2023

Could you please sign your commit @jprendes ?

Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
@jprendes
Copy link
Contributor Author

sorry, changed laptops and hadn't set up git correctly!
it's signed now :-)

@Mossaka Mossaka merged commit fd40db4 into deislabs:main Sep 21, 2023
20 checks passed
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