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

ref(dev): Fix and expand .envrc checks #522

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Apr 3, 2024

This is a rewrite of our .envrc file to solve a few problems I ran into getting the repo set up:

  • We use information from a .python-version file, but there is no such file (and in fact we .gitignore any file with that name).
  • We rely on the user to have set their global Python version to the version we want. If they don't, a) we put the wrong version into the virtual environment, and b) installing requirements can't successfully complete.
  • There are prerequisite CLI packages upon which either we or our dependencies rely, which may or may not be installed on the user's machine. (In my case it was cmake, which is required in order to install onnx, but we also require docker and docker-compose in order to run typechecking and the dev server, respectively.)
  • Our check for the existence of the virtual environment wasn't detecting it even when it was there.

To solve these problems, this PR makes the following changes:

  • There is now a .python-version file and it is no longer .gitignored.
  • We now rely on the Python version in the virtual environment to be the right one, rather than relying on the user to set it. In order to get the right version into the virtual env, we first check that said Python version exists on the user's machine (and offer to install it if not), and then use that version to create the env.
  • We now check for the required CLI packages and offer to install any that are missing. (Originally I was trying to do this with a Brewfile, but it wasn't doing a good job handling the fact that I already had docker and docker-compose installed, so I switched to only trying to install whatever packages are missing.)
  • We now use -d rather than -f to check for the virtual env, since it's a directory.

Also, all four checks done by .envrc (Python version, CLI packages, virtual env, and pre-commit hooks) are now listed in the readme.

Notes:

  • Of the four checks, two (Python version and CLI packages) affect things outside of the repo, while the other two (virtual env and hooks) only make changes within the repo's confines. Accordingly, I've made .envrc get the user's buy-in before making the first two fixes but not before making the second two. I also made it so dependencies are automatically installed when the env is created.
  • One check I didn't add is one to make sure, if the virtual env is there, that it contains all of the right dependencies, but I did add a TODO so we remember to do that at some point.
  • I am not super bash-y, and my background is JS rather than python, so it's entirely possible that I've done parts of this in a ham-fisted way. I'd love to know a better way if so!

@lobsterkatie lobsterkatie marked this pull request as ready for review April 3, 2024 03:44
@corps
Copy link
Contributor

corps commented Apr 6, 2024

Hey!

I'm definitely not against these changes. To be clear, part of the reason we haven't improved .envrc is that I explicitly wanted to move us away from using local python at all.

If you take a look at Makefile you'll see that we basically manage our development environment via docker-compose using a separated environment that requires no local python.

I am not against local python, but here are things you should know:

  1. Many ML libraries either:
    a. Don't work on mac without setting up binary installations of things
    b. Do work on mac but also behave differently altogether due to version / linking differences.
    This is especially the case for torch and even some embeddings stuff.

  2. All base configuration and environment variables are configured correctly in docker-compose. You'll have ot be responsible for figuring out how to expose and keep the local environment in sync

  3. CI runs the docker-compose stack, so any changes introduced locally need to also pass configuration in that stack.

Broadly, if it helps you and you're willing to take responsibility for it, cool! My time likely won't be spent ensuring local python works, and that is just a fair warning.

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.

3 participants