-
Notifications
You must be signed in to change notification settings - Fork 9
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
base: develop
Are you sure you want to change the base?
Conversation
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.
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.
tk-dev \ | ||
uuid-dev \ | ||
wget && \ | ||
rm -rf /var/lib/apt/lists/* |
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.
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
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.
i'm sure tha tmost of the image was from the ubuntu:16.04 base though? i think that's like 600MB or something
{ | ||
"username": "__USER__", | ||
"algoname": "__ALGO__", | ||
"language": "rust" |
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.
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).
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.
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.
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.
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
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.
Upgrading the compiler would be:
- rebuild the rust package set (assuming it's always pulling the latest)
- 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!")
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.
I interpret that as: I don't have to introduce any sort of version in this algorithmia.conf. Is that correct?
libraries/rust-buildtime/Dockerfile
Outdated
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 \ |
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.
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)
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.
@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".
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
Not sure I see the C++ stuff in here? If it was extra work I might rather leave it out from this PR |
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.
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
I believe the last commits make this much closer to something we could merge. I believe these are the remaining pieces:
|
- 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"'
(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.