-
Notifications
You must be signed in to change notification settings - Fork 24
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 Sail DevContainer Configuration #405
base: main
Are you sure you want to change the base?
Conversation
new file: .devcontainer/devcontainer.json
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.
@bcncpp Thanks for your contribution! I left some comments below. Happy to discuss!
RUN pip install --upgrade pip \ | ||
&& pip install maturin hatch | ||
|
||
# Set working directory |
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.
Let's remove the whitespace before the comment.
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.
let's remove the comment
@@ -0,0 +1,16 @@ | |||
{ | |||
"name": "Python 3.10 DevContainer", |
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.
Can we name it "Sail DevContainer"?
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.
Yes, gonna correct this
"ms-python.python", | ||
"ms-python.vscode-pylance" | ||
], | ||
"postCreateCommand": "uv sync && cargo build -p sail-cli --profile dev --bins", |
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.
We do not use uv
directly since the development involves multiple Python virtualenvs managed by hatch
. hatch
uses uv
or pip
internally to manage dependencies.
Maybe we should uv sync
here?
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.
As developer i would like to have all depedendencies synced here. How do I use hatch to have the deps on sync?
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.
Hatch keeps the dependency in sync automatically when running a hatch
command, so we don't need to sync the dependencies explicitly.
"extensions": [ | ||
"ms-python.python", | ||
"ms-python.vscode-pylance" | ||
], |
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.
"rust-lang.rust-analyzer"
could be a useful extension to have as well.
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.
Good catch. Gonna add
RUN curl -fsSL https://ziglang.org/builds/zig-linux-x86_64-$ZIG_VERSION.tar.xz | tar -xvJf - -C /usr/local/bin | ||
|
||
|
||
RUN wget -O - https://apt.corretto.aws/corretto.key | sudo gpg --dearmor -o /usr/share/keyrings/corretto-keyring.gpg && \ |
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.
Instead of installing Java, Rust, and Node in the Dockerfile, could we use features to install the required libraries? This may result in less code to maintain within Sail.
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.
Yes, i didn't know about features.
# Ensure the path is set for rust | ||
ENV PATH="/home/$USERNAME/.cargo/bin:$NVM_DIR/versions/node/${NODE_VERSION}/bin:/usr/local/bin:$PATH" | ||
RUN rustup toolchain install stable --component rustfmt && rustup toolchain install nightly --component rustfmt | ||
RUN cargo install maturin |
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.
We don't need cargo install maturin
since Maturin is also installed via pip
.
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.
Gonna remove
ARG PYSPARK_VERSION=3.5.4 | ||
RUN python3 -m pip install --no-cache-dir "pyspark[connect]==${PYSPARK_VERSION}" |
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 wonder if it's possible to install all the dependencies of the Hatch default
environment (defined in pyproject.toml
), without listing the dependencies in Dockerfile. This could be future work though.
Some background: we use hatch
and virtualenvs so the global Python installation is never used during development. The Hatch virtualenvs are stored in the .venvs
directory in the project root.
I assume we cannot just mount .venvs
directory inside the dev container since the packages installed on the host may have a different OS from the container (e.g. macOS vs Ubuntu). Probably we need to reevaluate how to make the Hatch setup play nicely with dev containers.
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 will give a try
RUN wget -qO- https://get.pnpm.io/install.sh | ENV="$HOME/.bashrc" SHELL="$(which bash)" bash - | ||
|
||
|
||
# Install Maturin (Python package), Hatch, and Protocol Buffers |
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.
"Protocol Buffers" is not installed via pip
so the comment needs to be changed.
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 remove the comment,
I simplified the PR title since it will appear as the final commit message. But thanks for the detailed description for the PR! |
Description [PR] Add Rust, Amazon Corretto 17, Python 3.10, and Maturin to Sail DevContainer Configuration
This PR introduces updates to the Sail DevContainer configuration to include the installation of essential development tools. It modifies/updates the following:
These updates will ensure that any new developer setting up the container will have all necessary dependencies pre-installed, reducing setup time and ensuring a smooth development experience.
Changes Made:
devcontainer.json
to include relevant extensions or environment configurations.Dockerfile
:These changes provide a ready-to-use development environment for any developer who clones the repository, ensuring all the required tools and dependencies are available.
Test Plan:
To ensure that the changes work properly, follow these steps:
Use the following command to build the container:
docker build -t devcontainer .