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

Add dev container support for project #704

Conversation

Joshmantova
Copy link
Contributor

Currently, there's a contributor.md file that looks to be out of date. I think it's confusing for new contributors to join and meaningfully contribute to the project because getting set up is somewhat complicated. This introduces a VSCode dev container solution for all developers to share a dev environment including proper linting, requirements, and extensions. I think this will make it easier for additional contributors to join without friction and improve the project. Let me know if there are any other customizations you'd want developers to be set up with for this project. Because this repo is on github, this will also allows anyone to develop from the web browser using Github codespaces and still have a proper dev environment set up.

@okhat
Copy link
Collaborator

okhat commented Mar 23, 2024

This is so cool! Thank you @Joshmantova — I have no experience with this type of container. Any reason I should wait before merging this? It looks reasonable to me.

@josh-mantovani
Copy link

This is so cool! Thank you @Joshmantova — I have no experience with this type of container. Any reason I should wait before merging this? It looks reasonable to me.

Nope! It's good to go unless you have additional suggestions for developer environments. We can always add to it later too. Now that I know you're interested in this, I'll throw up another pr to update the contributor documentation with further instructions.

@Joshmantova
Copy link
Contributor Author

Joshmantova commented Mar 24, 2024

This is so cool! Thank you @Joshmantova — I have no experience with this type of container. Any reason I should wait before merging this? It looks reasonable to me.

I realized there were pre-commit hooks in this project so I added those as well. Had to add pre-commit as a dependency as well. Also changed pre-commit python version to 3.11 (this is also what the devcontainer will load in).

"ghcr.io/devcontainers/features/github-cli:1": {},
"ghcr.io/schlich/devcontainer-features/powerlevel10k:1": {}
},
"image": "mcr.microsoft.com/devcontainers/python:3.11",
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 uses Python 3.11 - would the team like me to use 3.9 instead?

@@ -8,7 +8,7 @@ on:
types: [opened, synchronize, reopened]

env:
POETRY_VERSION: "1.6.1"
POETRY_VERSION: "1.7.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the poetry lock file, it looks like it's getting compiled with 1.7.1 - I'd recommend updating the version in CI to reflect what developers are using locally.

.github/workflows/precommits_check.yml Outdated Show resolved Hide resolved
@isaacbmiller
Copy link
Collaborator

This looks great! Thanks for doing this work.

How hard would it be to create a CI step to make sure the container runs properly? If its too difficult, let me know and I can merge this in.

@josh-mantovani
Copy link

This looks great! Thanks for doing this work.

How hard would it be to create a CI step to make sure the container runs properly? If its too difficult, let me know and I can merge this in.

@isaacbmiller - That's a great idea! Given that it takes a few minutes to build the container, we'd probably only want to trigger the CI workflow if the dev container files have changed. I'll get working on that but I'd love for it to be a fast follow so I don't need to keep resolving merge conflicts. Given that the changes here only affect internal developers using dev containers, which is currently only myself, I think it's safe to merge this in and bring in the CI step afterward. What do you think about that plan?

@Joshmantova
Copy link
Contributor Author

In response to some of @thomasahle 's comments in Discord, I decided to downgrade the dev container to python3.9 from 3.11. He pointed out that backward compatibility is essential and because of that, we should focus on testing using 3.9 rather than 3.11. If we're going to focus on 3.9, we should also recommend internal developers use 3.9 and use that in the dev container. Now there are no substantive changes to CI and the only changes here are relevant to the dev container. This comes with the exception of upgrading the Poetry version used in CI to reflect the version developers are using locally to compile the lock file.

@isaacbmiller isaacbmiller merged commit fb52b19 into stanfordnlp:main Mar 28, 2024
4 checks passed
@Joshmantova Joshmantova deleted the joshmantovani/add-dev-container-support branch March 28, 2024 16:38
@ammirsm
Copy link
Collaborator

ammirsm commented Apr 16, 2024

Hey folks, when you set this one up did you able to run the notebooks locally?

intro.ipynb for example?

@Joshmantova
Copy link
Contributor Author

Hey folks, when you set this one up did you able to run the notebooks locally?

intro.ipynb for example?

@ammirsm Works for me! When I ran the first cell, VSCode asked me to install an additional extension automatically. Make sure you have the correct venv selected for your notebook. If you're getting an error, could you send me the error either in discord or create an issue with the error and tag me in it?

@ammirsm
Copy link
Collaborator

ammirsm commented Apr 16, 2024

Is that running all blocks with cache?

Feel free to shoot me dm on discord with the same username on here

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.

5 participants