-
-
Notifications
You must be signed in to change notification settings - Fork 493
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
Adds basic devcontainer for #1057 #1532
Conversation
@jacoverster That is awesome. The scene test also fails locally for us. It is an unresolved issue. See #643. I think you do not need to resolve it for the dev container to be included in the next release. |
Thanks @rodja, this is tested on Windows 11 with WSL2. Would be good if someone can test on other OS setups too and provide feedback. |
fbcfd01
to
babf6c4
Compare
Okay, stupid question time. I cloned your repo, VS Code prompts to launch dev container, all good. Now I am in the container and feel like I am missing something basic. I should be able to launch the tests by executing
Also, using the built in VS Code testing UI. I am getting
|
Hi @natankeddem, with the container the testing now also happens inside the poetry virtual environment. You need to run |
For me (on mac, M1) the devcontainer does not build:
Expand
|
@jacoverster Would it not be more convenient to istall it not in a virtual env? It's a container so installing it "globally" should be fine and would simplify usage. |
Seems like Google Chrome is not available for Debian ARM processor yet. Because I the Apple M1/M2 chips are ARM based chrome can not be installed. |
Maybe it's possible to switch to chromium-browser? |
It looks like you can access the architecture info during build so we should be able to install processor specific packages. |
@rodja can you maybe modify your Dockerfile to a simplified version like this and populate the RUN section under install google chrome for ARM until it builds. Once it builds, add back the rest of the Dockerfile and check if the tests pass?
|
The chromedriver will probably also need to change. All the chrome and chromedriver build versions are available here so maybe we could install it the same way? |
@jacoverster it seems like there is no chrome package available for ARM: https://stackoverflow.com/a/71174097/364388 |
@rodja I tried following the best practices described in this discussion on the poetry repo and summarized in this blog post by one of the poetry contributors. Would it be possible to completely remove poetry and just install from the pytproject.toml file using pip? |
I've done some experiments/modifications with the Dockerfile to use chromium instead of chrome: FROM python:3.8-slim
# Chrome and chromedriver versions must match as per
# https://googlechromelabs.github.io/chrome-for-testing/#stable
ARG CHROME_VERSION=116.0.5845.96
ARG USERNAME=vscode
ARG USER_UID=1000
ARG USER_GID=$USER_UID
ENV POETRY_VERSION=1.6.1 \
PATH="/home/$USERNAME/.local/bin:$PATH" \
POETRY_NO_INTERACTION=1 \
POETRY_VIRTUALENVS_IN_PROJECT=1 \
POETRY_VIRTUALENVS_CREATE=1 \
POETRY_CACHE_DIR=/tmp/poetry_cache \
DISPLAY=:99 \
DEBIAN_FRONTEND=noninteractive
# Create remote user
RUN groupadd --gid $USER_GID $USERNAME \
&& useradd --uid $USER_UID --gid $USER_GID -m $USERNAME \
&& apt-get update \
&& apt-get install -y sudo git wget gnupg unzip curl build-essential \
&& echo $USERNAME ALL=\(root\) NOPASSWD:ALL > /etc/sudoers.d/$USERNAME \
&& chmod 0440 /etc/sudoers.d/$USERNAME \
&& rm -rf /var/lib/apt/lists/*
# Install webbrowser and webdriver
RUN apt-get update \
&& apt-get install -y chromium chromium-driver \
&& rm -rf /var/lib/apt/lists/*
USER $USERNAME
WORKDIR /workspaces/nicegui
RUN pip install -U pip && pip install poetry==$POETRY_VERSION
COPY ./pyproject.toml ./poetry.lock* main.py ./
RUN poetry install --all-extras
CMD python -m debugpy --listen 5678 main.py It starts up fine but the tests failed. The webdriver stuff in |
Ok great, I'll modify the Dockerfile to accommodate these changes towards being more platform-specific in the meantime. |
Coming in as just a "user" I feel like it might be nice if somehow we could get the VS Code testing GUI working with this? I understand if there are more complex issues at play that might not allow this functionality. Also, I am running into some errors following your instructions; it seems like there might be some missing dependencies?
Making the venv also partially removes the inherent advantage of using a container which is that it is a fully encapsulated/reproducible environment. Now you are creating/exposing a venv folder which was not necessarily part of your container build routine that may be polluted in some way outside the scope of the container.
|
@natankeddem to disable the venv just modify the env variables:
And be sure to delete the .venv folder in your project directory, otherwise poetry will detect it and use it. I've pushed the change to the PR, so just pull them. |
This is giving me a permissions error:
|
@rodja I noticed that the |
Just have a look at the updated Dockerfile, the |
Sorry @natankeddem I forgot to add the commit to the PR, will add now. |
Okay just let me know when things are in a state where you feel comfortable with me testing things out more. |
Please continue, I pushed the commit. |
Okay, container builds successfully now. I got dependency errors with
|
@natankeddem as per the current setup you first need to install nicegui locally with If you want to change the poetry installation with something like |
I suggest we update the relevant contribution/test documentation after this has been thoroughly tested @rodja. |
Makes sense, I am all for removing additional steps if you can. To get this to work I had to make a venv manually then select it in the dev container then |
https://askubuntu.com/a/1120102 |
This is strange. To further simplify things we can also consider moving away from the |
This reverts commit 423d83c.
If I remove the |
to fix pytest on macos arm (M1)
I got the tests working on mac arm64 (M1). I just needed to configure the binary to chromium. See c8a6eee. |
So enabling rosetta and switching to amd64 did get things to work but too slowly it sounds like? That discussion on https://github.com/browserless/chrome/ also had some ideas on downloading and matching ARM driver versions. I haven't really dug in, but is that any help to you guys on getting M1 support working? It says to get drivers from here: |
From my point of view we are almost ready (for a first "basic devcontainer"):
|
I've compared built sizes here and
I've made another change to the settings in
I'm going on a few days of leave today. Will tackle this when I get back. |
One other images we can consider is the official vscode one. It has some nice benefits like built in remote user, security patches, multi-arch support, etc. Just putting it on the table as a potential option as well. |
Interesting. I'll try it out. |
Have a look at the linked documentation and the additional features you can add to your container - might be something useful there for you. |
Should the extensions in |
Yes that would be good! |
While the official python container from vscode looks interesting it may take some time and thinking to figure out how we want to do this. I would like to do this in a separate PR. This one is already quite complex and with 60+ comments a long time in the making. |
Great, thanks for wrapping up the docs @rodja. Glad to get this merged. |
Continuation of #1057. Adding a basic VSCode devcontainer with robust chrome and chromedriver installation.
All tests pass except those under tests/test_scene.py.
Next steps: