MB-10413: Convert to using pipenv to manage dependencies#84
Conversation
ahobson
force-pushed
the
adh-pipenv
branch
4 times, most recently
from
9895ff0 to
7e8d0fa
Compare
ahobson
changed the title
ronaktruss
left a comment
ronaktruss
left a comment
There was a problem hiding this comment.
LGTM! Tested using nix.
| When setting up for the first time, before you run `direnv allow`, run | ||
|
|
||
| ```shell | ||
| make install_tools |
There was a problem hiding this comment.
I had to update my pyenv with instructions from make install_tools because Python 3.9.6 was not in the available list of installs.
Installing python 3.9.6 ...
python-build: definition not found: 3.9.6
See all available versions with `pyenv install --list'.
If the version you need is missing, try upgrading pyenv:
cd /Users/duncan/.pyenv/plugins/python-build/../.. && git pull && cd -
failed
There was a problem hiding this comment.
Oh whoops! What version of python should we be using?
There was a problem hiding this comment.
I think that matches what's in the Dockerfile but more recent than when I updated pyenv. I'm not sure how to address that in the brewfile if at all.
There was a problem hiding this comment.
Ah, I don't know either. I guess if you got it work, that's what counts?
There was a problem hiding this comment.
Weird, did you have pyenv previously? I wonder if we need to update the setup script to update pyenv if you already have it 🤔
duncan-truss
left a comment
duncan-truss
left a comment
There was a problem hiding this comment.
Dependencies setup seems to work for me and the docker container seems to be getting built alright.
I know we are saying the local docker setup is deprecated, I noticed that local_locust.py doesn't get copied to the container anymore so those commands fail.
felipe-lee
left a comment
felipe-lee
left a comment
There was a problem hiding this comment.
Haven't finished reviewing yet, but a few comments:
- https://github.com/transcom/milmove_load_testing/blob/adh-pipenv/.python-version can be deleted since it was for pyenv-virtualenv
- fresh could use a bit of cleaning to remove the pyenv-virtualenv stuff
@duncan-truss This was kinda discussed here. Since its a small change we can likely fix this, but we should probably be a bit more explicit on why we discourage/don't support docker. |
I removed the |
I can make those changes later today if you don't mind me pushing to your branch. I can also open a pr against this branch if you'd prefer. |
Push the to branch. We can hopefully merge this tomorrow |
| install_python_deps: ensure_venv ## Install all python dependencies/requirements | ||
| pip install -r requirements.txt | ||
| pip install -r requirements-dev.txt | ||
| install_python_deps: ## Install all python dependencies/requirements |
There was a problem hiding this comment.
Can we actually just get rid of this one? Direnv is handling pipenv no? If so, we could also get rid of the setup target since at that point it would just be an alias for ensure_pre_commit
There was a problem hiding this comment.
Since we don't have much experience with pipenv yet, can we leave it in to see if we ever need it? We can always remove it in the future
There was a problem hiding this comment.
Sounds good. In that case i think this is ready
I've made the changes for the fresh-brew stuff. Just one comment on the Makefile now, but otherwise I think it's ready. |
4 participants
Description
Pipenv lets us manage dependencies and creates a lock file so we can have reproducible builds.
In addition,
direnvsupports it natively and will install the dependencies and manage the virtualenv for us.I also took the opportunity to get the pre-commit in sync with the dependencies and use a
pyproject.tomlFinally, I fixed the tests and enabled them in CI
Reviewer Notes
I've tested this out with nix and as much as I can with brew, although I don't use it extensively, so any validation you all can do would be great
Setup
Check out the code and try to follow the README to get your environment set up