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

Rust and C++ package sets #70

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Rust and C++ package sets #70

wants to merge 2 commits into from

Conversation

anowell
Copy link
Contributor

@anowell anowell commented Feb 18, 2019

(disclaimer: low priority, no pressing requirement to CR or merge)

[Feb 2020 Update]
This PR was overhauled Feb 2020 to be more inline with other packageset design decisions. It was successfully validated with the packageset validator script, but hasn't actually been installed into any cluster.

Copy link
Contributor Author

@anowell anowell left a comment

Choose a reason for hiding this comment

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

2 other comments that don't fit inline anywhere:

  • runtime image? I'm not sure what to do here. Rust algos will need libssl (for algo client), but otherwise, it's completely dependent on what libs the algorithm used. In theory, that could be almost anything (e.g. there are even tensorflow bindings in rust), in practice, almost nobody uses rust on our platform. 🤷‍♂️
  • C/C++. I've demonstrated that it's possible to build C/C++ code in this package set. In theory, I could create a separate template that stubs out the C/C++ build pieces to make it easier to do, but I'm currently staying away from that because an official decision to support C/C++ deserves more thought than just hacking out a hybrid rust+cpp template.

languages/rust/config.json Show resolved Hide resolved
languages/rust/template/Cargo.toml Outdated Show resolved Hide resolved
languages/rust/template/src/lib.rs Outdated Show resolved Hide resolved
tk-dev \
uuid-dev \
wget && \
rm -rf /var/lib/apt/lists/*
Copy link
Contributor Author

@anowell anowell Feb 18, 2019

Choose a reason for hiding this comment

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

I'm really not sure what a good set of build deps for the rust image are. build-essential and libssl-dev are probably the bare minimum, and then it depends on how many lib* packages the algorithm tries to use. I basically stole the python build set as it seemed a pretty reasonable set. I did end up with a 1.19GB rust-buildtime image when using the ipa_packageset_builder script

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm sure tha tmost of the image was from the ubuntu:16.04 base though? i think that's like 600MB or something

libraries/rust-buildtime/Dockerfile Outdated Show resolved Hide resolved
{
"username": "__USER__",
"algoname": "__ALGO__",
"language": "rust"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should the lang name be versioned? Rust releases a new backward compatible release every 6 weeks, and in general, I'd expect that old algos can easily bump to a new compiler at least once every 6 months (the ecosystem really doesn't tend to support old compilers well for much longer than that - our current rust support is hard to use because any attempt to use a package published in the last 6 months is likely to require a newer version of rustc).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be versioned, as even though every release states it's BWC - sometimes there are some minor things (jmalloc?) that aren't and could lead to undefined behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jmalloc was technically backward compatible. But yes, there are exceptions, usually because of compiler soundness bugs, but as someone who has authored 15+ rust projects over 4 years, never once has a stable compiler upgrade broke my old code; but not upgrading has made it so I couldn't add/update deps.

So as long as there is a straight-forward way to upgrade the compiler for an algorithm every quarter or two, then versioning the rust packageset sounds fine to me

Copy link
Contributor

Choose a reason for hiding this comment

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

Upgrading the compiler would be:

  1. rebuild the rust package set (assuming it's always pulling the latest)
  2. upgrading the version of the packageset in use for an algorithm to the latest build (if you set packageSetName when doing a create/update algorithm API call this will happen - we don't have a way yet to show something in the UI like "by the way you can upgrade your base image!")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I interpret that as: I don't have to introduce any sort of version in this algorithmia.conf. Is that correct?

RUN set -ex \
&& wget https://static.rust-lang.org/rustup/archive/1.16.0/x86_64-unknown-linux-gnu/rustup-init \
&& chmod +x rustup-init \
&& ./rustup-init -y --no-modify-path \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line installs the latest stable version of rust (not to be confused with the line 2 lines up that installs a specific version of rustup). Depending on how we want to version and update rust package sets, we may need to pin this line to a specific version (e.g. --default-toolchain 1.32.0, the current stable)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pmcq is the 👍 and ask to hard-code the rustc version in the Dockerfile, or that it's fine to just have this Dockerfile install "latest rustc".

@pmcq
Copy link
Contributor

pmcq commented Feb 22, 2019

* [ ]  runtime image? I'm not sure what to do here. Rust algos will need libssl (for algo client), but otherwise, it's completely dependent on what libs the algorithm used. In theory, that could be almost anything (e.g. there are even tensorflow bindings in rust), in practice, almost nobody uses rust on our platform. 🤷‍♂️

My thought is that the base image should be what it is now - small and with the things we know are common. Beyond that we could be creating packagesets as needed (e.g. rust + ffmpeg bindings). Ideally we'd have the + whatever extend from the same base image but that's WIP still.

* [ ]  C/C++. I've demonstrated that it's possible to build C/C++ code in this package set. In theory, I could create a separate template that stubs out the C/C++ build pieces to make it easier to do, but I'm currently staying away from that because an official decision to support C/C++ deserves more thought than just hacking out a hybrid rust+cpp template.

Not sure I see the C++ stuff in here? If it was extra work I might rather leave it out from this PR

Copy link
Contributor

@pmcq pmcq left a comment

Choose a reason for hiding this comment

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

overall seems like a good strategy but I don't see how an algorithm would actually get built still? i think just the build script is missing getting copied into the buildtime image. but if that's good then i feel like we should just try out it cause why not... i don't think i can fairly judge without trying to kick the tires of it haha

@anowell
Copy link
Contributor Author

anowell commented Apr 6, 2019

I believe the last commits make this much closer to something we could merge. I believe these are the remaining pieces:

  • Accept or reject the design choice of exposing fn main and a function to register the apply function. Pages 9-11 of this doc build the case for this approach
  • Nail down versioning story for rust packageset
    • fixed version or latest stable in Dockerfile?
    • Should the algorithmia.conf file have some sort of version?
  • Testing - need to re-verify packageset builds, and then actually figure out a good way to test the containerized build/run. The process felt very clunky the last time I tried.
  • Cut a non-beta 3.0 release of the rust client (just waiting on any feedback on the API for registering the handler which has reference documentation here) and use it in this template
  • Figure out how to ship this.

Anthony Nowell added 2 commits February 1, 2020 22:41
- Removes entrypoint macros in favor of exposing `fn main` directly with a single helper that registers the
apply function (wiring up the langpack spec behind the scenes)
- Update algorithmia client to version that provides the handler logic
- Switched from library to binary rust project by default. Nothing stops
advanced users from reorganizing.
- Reduced the dependencies needed in the template

Tested with:

$ tools/packageset_validator.py -t dependency -s rust -n rust -d cpp-rust -b 'ubuntu:18.04'

$ curl localhost:9999 -H 'Content-Type: application/json' -d '{"name": "Anthony"}'
Tested with:

$ tools/packageset_validator.py -t dependency -s rust -n cpp-rust -d cpp-rust -b 'ubuntu:18.04'

$ curl localhost:9999 -H 'Content-Type: application/json' -d '"Anthony"'
@anowell anowell changed the title Adding rust-buildtime and new lang template for IPA Rust and C++ package sets Mar 10, 2020
Base automatically changed from master to develop February 2, 2021 19:32
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