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

Implement devcontainer for NiceGUI dev environment #1057

Closed
wants to merge 9 commits into from

Conversation

xec-abailey
Copy link
Contributor

@xec-abailey xec-abailey commented Jun 21, 2023

Per the conversation in #1018 the goal of this PR is to implement devcontainers for the development environment to make contribution easier.

Instructions:

  • Clone repo
  • Open vscode
  • ctrl-shift-P -> Devcontainers: Rebuild and Reopen in Container

OR

  • Codespaces
  • "Create new codespace on devcontainer"

It will build to the development target, install system dependencies (including webdriver) and open into the root project folder.

Commentary

  • Added another target in the multi-stage dockerfile for chromium only since it's a critical and complex system dependency
  • Configured poetry to install to the .venv folder in the root project folder. This is so much more intuitive to me than the arbitrary system path that poetry uses by default. Kind of follows the logic that node_modules, or rust crates follow in terms of dependency install location
  • switched the python docker image over from slim to standard shared tag because the slim images are annoying to work with if you are actually going to develop inside them. Still can use slim for release though

@xec-abailey
Copy link
Contributor Author

xec-abailey commented Jun 21, 2023

@zauberzeug-dev @rodja not sure who to ping on this but

Everything is looking good so far, webdriver appears to be working fine and the tests appear to render in the browser fine. Issue is that screen.should_contain is not finding anything and all screenshots of the local ui are blank.

To help myself figure out this issue I added test_selenium.py and have been debugging around to identify issues. Haven't figured out what the deal is but any advice is appreciated. You should be able to just open in a codespace or clone this branch and open in devcontainer to get right to where I'm at.

Cheers!

@xec-abailey
Copy link
Contributor Author

xec-abailey commented Jun 23, 2023

Hacked at this again for a bit - driver is definitely working, can take screenshots and do normal selenium stuff in the dev environment. Issues are mainly arising when trying to use screen and render the contents of the test - find is not returning any of the content and screenshots can't be taken for some reason (although they work fine for websites). Not sure what I'm doing wrong there I'll keep trying though

@jacoverster
Copy link
Contributor

jacoverster commented Jun 27, 2023

Hey @xec-abailey, good work! I also hacked away at this, used both your and my own containers but and ran into similar issues as you described. I used WSL2 on Windows 11 and Windows 11 native to test. I suspect someone with in-depth experience with these drivers might need to lend a hand here.

Some comments of things that I kept running into:

  • I had to install the httpx package as is it is required by the tests but not part of the listed requirements.
  • Not sure if the poetry install is propagated through the docker image properly (also seen when using development.dockerfile). nicegui is not found and I have to run pip install -e . again. I'm not used to using poetry inside Docker, so it might just be my ignorance here.
  • Just a question out of interest (and ignorance!): Do we need poetry inside Docker? Would a pip install not do the same or is the poetry.lock file critical?

Some suggestions:

  • adding pip install -e . as a "postCreateCommand" to devcontainer.json
  • The Python image is Debian-based so consider using the Debian chromium-driver package instead of the install from zip
  • In the multi-stage Dockerfile - moving the test stage before development, this lets you target that stage during CI builds and avoids dev dep installs to save some time
  • Add tests dependency install to test stage as per the test README pip install -r tests/requirements.txt
  • add a .docker folder to the repo to store all docker-related files

@xec-abailey
Copy link
Contributor Author

Awesome, glad to have a second set of eyes here!

Do we need poetry inside Docker? Would a pip install not do the same or is the poetry.lock file critical?

Yes! Poetry is critical in this case for locking down dependencies - the lock file guarantees consistency in installation between developers and deployments. The way that the current dev environment uses both pip and poetry is awkward as you can just use poetry for all dependency specification. Under the hood poetry uses pip to install everything - so again it's not needed to do them separately. If you look in my branch you'll see I just added all of the test dependencies to the dev group and the development target calls "poetry install --no-root" which will install the dependencies in all groups to the virtual env.

pip install -e . in postCreateCommand

Per the above this should therefore not be necessary!

In the multi-stage Dockerfile - moving the test stage before development, this lets you target that stage during CI builds and avoids dev dep installs to save some time

You can build to a target regardless of order in the Dockerfile! I haven't finalized the test target in my dockerfile as of yet but testing will require all of the dev dependencies anyway because tests are inherently in the dev context. So in all likelihood it should end up being a downstream target of "development". To follow your logic though - I'm pretty sure there will be some build-time efficiencies there that can be iterated on once the basic structure is setup.

@xec-abailey
Copy link
Contributor Author

The Python image is Debian-based so consider using the Debian chromium-driver package instead of the install from zip

Great suggestion here I'm going to give this a shot!

@rodja
Copy link
Member

rodja commented Jun 29, 2023

Maybe the pull request #1110 fixes the selenium execution?

@rodja
Copy link
Member

rodja commented Jul 9, 2023

@xec-abailey and @jacoverster have you found time to re-test running selenium? We really like this PR and would be happy to become the basis for future contributors.

@xec-abailey
Copy link
Contributor Author

@xec-abailey and @jacoverster have you found time to re-test running selenium? We really like this PR and would be happy to become the basis for future contributors.

I will have some time this week, summer-time has gotten busy on my end. Definitely is close - need to review but there is some strange behavior related to the Screen class starting the webserver and the selenium driver.

Apologies for the delay!

@jacoverster
Copy link
Contributor

jacoverster commented Jul 12, 2023

I'm happy to assist with testing on Windows 11 and WSL2 Ubuntu on my side once your get it running @xec-abailey

@falkoschindler
Copy link
Contributor

Hi @xec-abailey and @jacoverster, just wanted to hear if you're still working on this PR.
We're still very interested in providing a proper dev container for NiceGUI.

@jacoverster
Copy link
Contributor

Hi @rodja, I'll have some time this week to look at this again. It seems like getting the Chrome Web driver to run properly is the bottleneck so I'll see if I can get that to run reliably in Docker and at least execute the tests.

@jacoverster
Copy link
Contributor

Hi @rodja, I've made some good progress with this. The main issues with chrome and chromedriver seems to be that the versions need to match as per their release documentation. They are on separate release cycles so getting the latest versions of both does not guarantee compatibility. I've managed to get the tests to run in an updated Dockerfile. I'll make the changes to this PR over the next few days.

@xec-abailey
Copy link
Contributor Author

Hi @rodja, I've made some good progress with this. The main issues with chrome and chromedriver seems to be that the versions need to match as per their release documentation. They are on separate release cycles so getting the latest versions of both does not guarantee compatibility. I've managed to get the tests to run in an updated Dockerfile. I'll make the changes to this PR over the next few days.

That was my blocker as well, thanks for keeping on it!

@falkoschindler
Copy link
Contributor

I favor of PR #1532 I'll close this one.

rodja added a commit that referenced this pull request Sep 23, 2023
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

4 participants